* [PATCH 1/2] iommu/arm-smmu: Track context bank state
@ 2017-07-18 12:44 ` Robin Murphy
0 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2017-07-18 12:44 UTC (permalink / raw)
To: will.deacon-5wv7dgnIgG8, joro-zLv9SwRftAIdnm+yROfE0A
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Echoing what we do for Stream Map Entries, maintain a software shadow
state for context bank configuration. With this in place, we are mere
moments away from blissfully easy suspend/resume support.
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
Since the runtime PM discussion has come back again, I figured it was
probably about time to finish off my plan for system PM. Lightly tested
on Juno (MMU-401) with hibernation.
Robin.
drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++++------------------
1 file changed, 97 insertions(+), 62 deletions(-)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index bc89b4d6c043..86897b7b81d8 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -338,6 +338,13 @@ struct arm_smmu_smr {
bool valid;
};
+struct arm_smmu_cb {
+ u64 ttbr[2];
+ u32 tcr[2];
+ u32 mair[2];
+ struct arm_smmu_cfg *cfg;
+};
+
struct arm_smmu_master_cfg {
struct arm_smmu_device *smmu;
s16 smendx[];
@@ -380,6 +387,7 @@ struct arm_smmu_device {
u32 num_context_banks;
u32 num_s2_context_banks;
DECLARE_BITMAP(context_map, ARM_SMMU_MAX_CBS);
+ struct arm_smmu_cb *cbs;
atomic_t irptndx;
u32 num_mapping_groups;
@@ -768,17 +776,69 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
struct io_pgtable_cfg *pgtbl_cfg)
{
- u32 reg, reg2;
- u64 reg64;
- bool stage1;
struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
- struct arm_smmu_device *smmu = smmu_domain->smmu;
+ struct arm_smmu_cb *cb = &smmu_domain->smmu->cbs[cfg->cbndx];
+ bool stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
+
+ cb->cfg = cfg;
+
+ /* TTBCR */
+ if (stage1) {
+ if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
+ cb->tcr[0] = pgtbl_cfg->arm_v7s_cfg.tcr;
+ } else {
+ cb->tcr[0] = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
+ cb->tcr[1] = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
+ cb->tcr[1] |= TTBCR2_SEP_UPSTREAM;
+ if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
+ cb->tcr[1] |= TTBCR2_AS;
+ }
+ } else {
+ cb->tcr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
+ }
+
+ /* TTBRs */
+ if (stage1) {
+ if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
+ cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
+ cb->ttbr[1] = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
+ } else {
+ cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+ cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
+ }
+ } else {
+ cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
+ }
+
+ /* MAIRs (stage-1 only) */
+ if (stage1) {
+ if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
+ cb->mair[0] = pgtbl_cfg->arm_v7s_cfg.prrr;
+ cb->mair[1] = pgtbl_cfg->arm_v7s_cfg.nmrr;
+ } else {
+ cb->mair[0] = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
+ cb->mair[1] = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
+ }
+ }
+}
+
+static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
+{
+ u32 reg;
+ bool stage1;
+ struct arm_smmu_cb *cb = &smmu->cbs[idx];
+ struct arm_smmu_cfg *cfg = cb->cfg;
+ struct arm_smmu_cfg default_cfg = {0};
void __iomem *cb_base, *gr1_base;
+ if (!cfg)
+ cfg = &default_cfg;
+
gr1_base = ARM_SMMU_GR1(smmu);
stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
- cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
+ cb_base = ARM_SMMU_CB(smmu, idx);
+ /* CBA2R */
if (smmu->version > ARM_SMMU_V1) {
if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
reg = CBA2R_RW64_64BIT;
@@ -788,7 +848,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
if (smmu->features & ARM_SMMU_FEAT_VMID16)
reg |= cfg->vmid << CBA2R_VMID_SHIFT;
- writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
+ writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(idx));
}
/* CBAR */
@@ -807,72 +867,43 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
/* 8-bit VMIDs live in CBAR */
reg |= cfg->vmid << CBAR_VMID_SHIFT;
}
- writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
+ writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(idx));
/*
* TTBCR
* We must write this before the TTBRs, since it determines the
* access behaviour of some fields (in particular, ASID[15:8]).
*/
- if (stage1) {
- if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
- reg = pgtbl_cfg->arm_v7s_cfg.tcr;
- reg2 = 0;
- } else {
- reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
- reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
- reg2 |= TTBCR2_SEP_UPSTREAM;
- if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
- reg2 |= TTBCR2_AS;
- }
- if (smmu->version > ARM_SMMU_V1)
- writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
- } else {
- reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
- }
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
+ if (stage1 && smmu->version > ARM_SMMU_V1)
+ writel_relaxed(cb->tcr[1], cb_base + ARM_SMMU_CB_TTBCR2);
+ writel_relaxed(cb->tcr[0], cb_base + ARM_SMMU_CB_TTBCR);
/* TTBRs */
- if (stage1) {
- if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
- reg = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0);
- reg = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1);
- writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
- } else {
- reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
- reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
- writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
- reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
- reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
- writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR1);
- }
+ if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
+ writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
+ writel_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
+ writel_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
} else {
- reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
- writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
+ writeq_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
+ writeq_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
}
/* MAIRs (stage-1 only) */
if (stage1) {
- if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
- reg = pgtbl_cfg->arm_v7s_cfg.prrr;
- reg2 = pgtbl_cfg->arm_v7s_cfg.nmrr;
- } else {
- reg = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
- reg2 = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
- }
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_S1_MAIR0);
- writel_relaxed(reg2, cb_base + ARM_SMMU_CB_S1_MAIR1);
+ writel_relaxed(cb->mair[0], cb_base + ARM_SMMU_CB_S1_MAIR0);
+ writel_relaxed(cb->mair[1], cb_base + ARM_SMMU_CB_S1_MAIR1);
}
/* SCTLR */
- reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
- if (stage1)
- reg |= SCTLR_S1_ASIDPNE;
-#ifdef __BIG_ENDIAN
- reg |= SCTLR_E;
-#endif
+ if (!cb->cfg) {
+ reg = 0;
+ } else {
+ reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
+ if (stage1)
+ reg |= SCTLR_S1_ASIDPNE;
+ if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
+ reg |= SCTLR_E;
+ }
writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
}
@@ -1035,6 +1066,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
/* Initialise the context bank with our page table cfg */
arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
+ arm_smmu_write_context_bank(smmu, cfg->cbndx);
/*
* Request context fault interrupt. Do this last to avoid the
@@ -1067,7 +1099,6 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
struct arm_smmu_device *smmu = smmu_domain->smmu;
struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
- void __iomem *cb_base;
int irq;
if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
@@ -1077,8 +1108,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
* Disable the context bank and free the page tables before freeing
* it.
*/
- cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
- writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
+ smmu->cbs[cfg->cbndx] = (struct arm_smmu_cb){0};
+ arm_smmu_write_context_bank(smmu, cfg->cbndx);
if (cfg->irptndx != INVALID_IRPTNDX) {
irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx];
@@ -1722,7 +1753,6 @@ static struct iommu_ops arm_smmu_ops = {
static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
{
void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
- void __iomem *cb_base;
int i;
u32 reg, major;
@@ -1758,8 +1788,9 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
/* Make sure all context banks are disabled and clear CB_FSR */
for (i = 0; i < smmu->num_context_banks; ++i) {
- cb_base = ARM_SMMU_CB(smmu, i);
- writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
+ void __iomem *cb_base = ARM_SMMU_CB(smmu, i);
+
+ arm_smmu_write_context_bank(smmu, i);
writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
/*
* Disable MMU-500's not-particularly-beneficial next-page
@@ -1964,6 +1995,10 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
smmu->cavium_id_base -= smmu->num_context_banks;
dev_notice(smmu->dev, "\tenabling workaround for Cavium erratum 27704\n");
}
+ smmu->cbs = devm_kcalloc(smmu->dev, smmu->num_context_banks,
+ sizeof(*smmu->cbs), GFP_KERNEL);
+ if (!smmu->cbs)
+ return -ENOMEM;
/* ID2 */
id = readl_relaxed(gr0_base + ARM_SMMU_GR0_ID2);
--
2.12.2.dirty
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 1/2] iommu/arm-smmu: Track context bank state
@ 2017-07-18 12:44 ` Robin Murphy
0 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2017-07-18 12:44 UTC (permalink / raw)
To: linux-arm-kernel
Echoing what we do for Stream Map Entries, maintain a software shadow
state for context bank configuration. With this in place, we are mere
moments away from blissfully easy suspend/resume support.
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
Since the runtime PM discussion has come back again, I figured it was
probably about time to finish off my plan for system PM. Lightly tested
on Juno (MMU-401) with hibernation.
Robin.
drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++++------------------
1 file changed, 97 insertions(+), 62 deletions(-)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index bc89b4d6c043..86897b7b81d8 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -338,6 +338,13 @@ struct arm_smmu_smr {
bool valid;
};
+struct arm_smmu_cb {
+ u64 ttbr[2];
+ u32 tcr[2];
+ u32 mair[2];
+ struct arm_smmu_cfg *cfg;
+};
+
struct arm_smmu_master_cfg {
struct arm_smmu_device *smmu;
s16 smendx[];
@@ -380,6 +387,7 @@ struct arm_smmu_device {
u32 num_context_banks;
u32 num_s2_context_banks;
DECLARE_BITMAP(context_map, ARM_SMMU_MAX_CBS);
+ struct arm_smmu_cb *cbs;
atomic_t irptndx;
u32 num_mapping_groups;
@@ -768,17 +776,69 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
struct io_pgtable_cfg *pgtbl_cfg)
{
- u32 reg, reg2;
- u64 reg64;
- bool stage1;
struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
- struct arm_smmu_device *smmu = smmu_domain->smmu;
+ struct arm_smmu_cb *cb = &smmu_domain->smmu->cbs[cfg->cbndx];
+ bool stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
+
+ cb->cfg = cfg;
+
+ /* TTBCR */
+ if (stage1) {
+ if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
+ cb->tcr[0] = pgtbl_cfg->arm_v7s_cfg.tcr;
+ } else {
+ cb->tcr[0] = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
+ cb->tcr[1] = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
+ cb->tcr[1] |= TTBCR2_SEP_UPSTREAM;
+ if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
+ cb->tcr[1] |= TTBCR2_AS;
+ }
+ } else {
+ cb->tcr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
+ }
+
+ /* TTBRs */
+ if (stage1) {
+ if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
+ cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
+ cb->ttbr[1] = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
+ } else {
+ cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+ cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
+ }
+ } else {
+ cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
+ }
+
+ /* MAIRs (stage-1 only) */
+ if (stage1) {
+ if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
+ cb->mair[0] = pgtbl_cfg->arm_v7s_cfg.prrr;
+ cb->mair[1] = pgtbl_cfg->arm_v7s_cfg.nmrr;
+ } else {
+ cb->mair[0] = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
+ cb->mair[1] = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
+ }
+ }
+}
+
+static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
+{
+ u32 reg;
+ bool stage1;
+ struct arm_smmu_cb *cb = &smmu->cbs[idx];
+ struct arm_smmu_cfg *cfg = cb->cfg;
+ struct arm_smmu_cfg default_cfg = {0};
void __iomem *cb_base, *gr1_base;
+ if (!cfg)
+ cfg = &default_cfg;
+
gr1_base = ARM_SMMU_GR1(smmu);
stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
- cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
+ cb_base = ARM_SMMU_CB(smmu, idx);
+ /* CBA2R */
if (smmu->version > ARM_SMMU_V1) {
if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
reg = CBA2R_RW64_64BIT;
@@ -788,7 +848,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
if (smmu->features & ARM_SMMU_FEAT_VMID16)
reg |= cfg->vmid << CBA2R_VMID_SHIFT;
- writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
+ writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(idx));
}
/* CBAR */
@@ -807,72 +867,43 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
/* 8-bit VMIDs live in CBAR */
reg |= cfg->vmid << CBAR_VMID_SHIFT;
}
- writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
+ writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(idx));
/*
* TTBCR
* We must write this before the TTBRs, since it determines the
* access behaviour of some fields (in particular, ASID[15:8]).
*/
- if (stage1) {
- if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
- reg = pgtbl_cfg->arm_v7s_cfg.tcr;
- reg2 = 0;
- } else {
- reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
- reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
- reg2 |= TTBCR2_SEP_UPSTREAM;
- if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
- reg2 |= TTBCR2_AS;
- }
- if (smmu->version > ARM_SMMU_V1)
- writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
- } else {
- reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
- }
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
+ if (stage1 && smmu->version > ARM_SMMU_V1)
+ writel_relaxed(cb->tcr[1], cb_base + ARM_SMMU_CB_TTBCR2);
+ writel_relaxed(cb->tcr[0], cb_base + ARM_SMMU_CB_TTBCR);
/* TTBRs */
- if (stage1) {
- if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
- reg = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0);
- reg = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1);
- writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
- } else {
- reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
- reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
- writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
- reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
- reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
- writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR1);
- }
+ if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
+ writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
+ writel_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
+ writel_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
} else {
- reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
- writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
+ writeq_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
+ writeq_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
}
/* MAIRs (stage-1 only) */
if (stage1) {
- if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
- reg = pgtbl_cfg->arm_v7s_cfg.prrr;
- reg2 = pgtbl_cfg->arm_v7s_cfg.nmrr;
- } else {
- reg = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
- reg2 = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
- }
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_S1_MAIR0);
- writel_relaxed(reg2, cb_base + ARM_SMMU_CB_S1_MAIR1);
+ writel_relaxed(cb->mair[0], cb_base + ARM_SMMU_CB_S1_MAIR0);
+ writel_relaxed(cb->mair[1], cb_base + ARM_SMMU_CB_S1_MAIR1);
}
/* SCTLR */
- reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
- if (stage1)
- reg |= SCTLR_S1_ASIDPNE;
-#ifdef __BIG_ENDIAN
- reg |= SCTLR_E;
-#endif
+ if (!cb->cfg) {
+ reg = 0;
+ } else {
+ reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
+ if (stage1)
+ reg |= SCTLR_S1_ASIDPNE;
+ if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
+ reg |= SCTLR_E;
+ }
writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
}
@@ -1035,6 +1066,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
/* Initialise the context bank with our page table cfg */
arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
+ arm_smmu_write_context_bank(smmu, cfg->cbndx);
/*
* Request context fault interrupt. Do this last to avoid the
@@ -1067,7 +1099,6 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
struct arm_smmu_device *smmu = smmu_domain->smmu;
struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
- void __iomem *cb_base;
int irq;
if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
@@ -1077,8 +1108,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
* Disable the context bank and free the page tables before freeing
* it.
*/
- cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
- writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
+ smmu->cbs[cfg->cbndx] = (struct arm_smmu_cb){0};
+ arm_smmu_write_context_bank(smmu, cfg->cbndx);
if (cfg->irptndx != INVALID_IRPTNDX) {
irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx];
@@ -1722,7 +1753,6 @@ static struct iommu_ops arm_smmu_ops = {
static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
{
void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
- void __iomem *cb_base;
int i;
u32 reg, major;
@@ -1758,8 +1788,9 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
/* Make sure all context banks are disabled and clear CB_FSR */
for (i = 0; i < smmu->num_context_banks; ++i) {
- cb_base = ARM_SMMU_CB(smmu, i);
- writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
+ void __iomem *cb_base = ARM_SMMU_CB(smmu, i);
+
+ arm_smmu_write_context_bank(smmu, i);
writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
/*
* Disable MMU-500's not-particularly-beneficial next-page
@@ -1964,6 +1995,10 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
smmu->cavium_id_base -= smmu->num_context_banks;
dev_notice(smmu->dev, "\tenabling workaround for Cavium erratum 27704\n");
}
+ smmu->cbs = devm_kcalloc(smmu->dev, smmu->num_context_banks,
+ sizeof(*smmu->cbs), GFP_KERNEL);
+ if (!smmu->cbs)
+ return -ENOMEM;
/* ID2 */
id = readl_relaxed(gr0_base + ARM_SMMU_GR0_ID2);
--
2.12.2.dirty
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] iommu/arm-smmu: Add system PM support
2017-07-18 12:44 ` Robin Murphy
@ 2017-07-18 12:44 ` Robin Murphy
-1 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2017-07-18 12:44 UTC (permalink / raw)
To: will.deacon-5wv7dgnIgG8, joro-zLv9SwRftAIdnm+yROfE0A
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
With all our hardware state tracked in such a way that we can naturally
restore it as part of the necessary reset, resuming is trivial, and
there's nothing to do on suspend at all.
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
drivers/iommu/arm-smmu.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 86897b7b81d8..0f5f06e9abfa 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -2356,10 +2356,22 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
return 0;
}
+static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
+{
+ struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+ arm_smmu_device_reset(smmu);
+ return 0;
+}
+
+
+static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
+
static struct platform_driver arm_smmu_driver = {
.driver = {
.name = "arm-smmu",
.of_match_table = of_match_ptr(arm_smmu_of_match),
+ .pm = &arm_smmu_pm_ops,
},
.probe = arm_smmu_device_probe,
.remove = arm_smmu_device_remove,
--
2.12.2.dirty
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] iommu/arm-smmu: Add system PM support
@ 2017-07-18 12:44 ` Robin Murphy
0 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2017-07-18 12:44 UTC (permalink / raw)
To: linux-arm-kernel
With all our hardware state tracked in such a way that we can naturally
restore it as part of the necessary reset, resuming is trivial, and
there's nothing to do on suspend at all.
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
drivers/iommu/arm-smmu.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 86897b7b81d8..0f5f06e9abfa 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -2356,10 +2356,22 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
return 0;
}
+static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
+{
+ struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+ arm_smmu_device_reset(smmu);
+ return 0;
+}
+
+
+static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
+
static struct platform_driver arm_smmu_driver = {
.driver = {
.name = "arm-smmu",
.of_match_table = of_match_ptr(arm_smmu_of_match),
+ .pm = &arm_smmu_pm_ops,
},
.probe = arm_smmu_device_probe,
.remove = arm_smmu_device_remove,
--
2.12.2.dirty
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] iommu/arm-smmu: Track context bank state
2017-07-18 12:44 ` Robin Murphy
@ 2017-07-19 2:40 ` Sricharan R
-1 siblings, 0 replies; 18+ messages in thread
From: Sricharan R @ 2017-07-19 2:40 UTC (permalink / raw)
To: Robin Murphy, will.deacon-5wv7dgnIgG8, joro-zLv9SwRftAIdnm+yROfE0A
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Hi Robin,
On 7/18/2017 6:14 PM, Robin Murphy wrote:
> Echoing what we do for Stream Map Entries, maintain a software shadow
> state for context bank configuration. With this in place, we are mere
> moments away from blissfully easy suspend/resume support.
>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>
> Since the runtime PM discussion has come back again, I figured it was
> probably about time to finish off my plan for system PM. Lightly tested
> on Juno (MMU-401) with hibernation.
>
> Robin.
>
> drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++++------------------
> 1 file changed, 97 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index bc89b4d6c043..86897b7b81d8 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -338,6 +338,13 @@ struct arm_smmu_smr {
> bool valid;
> };
>
> +struct arm_smmu_cb {
> + u64 ttbr[2];
> + u32 tcr[2];
> + u32 mair[2];
> + struct arm_smmu_cfg *cfg;
> +};
> +
When i was trying this sometime back [1], was
saving and using the pgtbl->cfg and domain->cfg for
restoring the context. But this looks correct to make
a actual shadow once and use it later all the time.
Will also try some testing today.
Reviewed-by: sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org
[1] https://patchwork.kernel.org/patch/9389717/
Regards,
Sricharan
> struct arm_smmu_master_cfg {
> struct arm_smmu_device *smmu;
> s16 smendx[];
> @@ -380,6 +387,7 @@ struct arm_smmu_device {
> u32 num_context_banks;
> u32 num_s2_context_banks;
> DECLARE_BITMAP(context_map, ARM_SMMU_MAX_CBS);
> + struct arm_smmu_cb *cbs;
> atomic_t irptndx;
>
> u32 num_mapping_groups;
> @@ -768,17 +776,69 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
> static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> struct io_pgtable_cfg *pgtbl_cfg)
> {
> - u32 reg, reg2;
> - u64 reg64;
> - bool stage1;
> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> - struct arm_smmu_device *smmu = smmu_domain->smmu;
> + struct arm_smmu_cb *cb = &smmu_domain->smmu->cbs[cfg->cbndx];
> + bool stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
> +
> + cb->cfg = cfg;
> +
> + /* TTBCR */
> + if (stage1) {
> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> + cb->tcr[0] = pgtbl_cfg->arm_v7s_cfg.tcr;
> + } else {
> + cb->tcr[0] = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
> + cb->tcr[1] = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
> + cb->tcr[1] |= TTBCR2_SEP_UPSTREAM;
> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> + cb->tcr[1] |= TTBCR2_AS;
> + }
> + } else {
> + cb->tcr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
> + }
> +
> + /* TTBRs */
> + if (stage1) {
> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> + cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
> + cb->ttbr[1] = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
> + } else {
> + cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
> + cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
> + }
> + } else {
> + cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
> + }
> +
> + /* MAIRs (stage-1 only) */
> + if (stage1) {
> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> + cb->mair[0] = pgtbl_cfg->arm_v7s_cfg.prrr;
> + cb->mair[1] = pgtbl_cfg->arm_v7s_cfg.nmrr;
> + } else {
> + cb->mair[0] = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
> + cb->mair[1] = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
> + }
> + }
> +}
> +
> +static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
> +{
> + u32 reg;
> + bool stage1;
> + struct arm_smmu_cb *cb = &smmu->cbs[idx];
> + struct arm_smmu_cfg *cfg = cb->cfg;
> + struct arm_smmu_cfg default_cfg = {0};
> void __iomem *cb_base, *gr1_base;
>
> + if (!cfg)
> + cfg = &default_cfg;
> +
> gr1_base = ARM_SMMU_GR1(smmu);
> stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
> + cb_base = ARM_SMMU_CB(smmu, idx);
>
> + /* CBA2R */
> if (smmu->version > ARM_SMMU_V1) {
> if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> reg = CBA2R_RW64_64BIT;
> @@ -788,7 +848,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> if (smmu->features & ARM_SMMU_FEAT_VMID16)
> reg |= cfg->vmid << CBA2R_VMID_SHIFT;
>
> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(idx));
> }
>
> /* CBAR */
> @@ -807,72 +867,43 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> /* 8-bit VMIDs live in CBAR */
> reg |= cfg->vmid << CBAR_VMID_SHIFT;
> }
> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(idx));
>
> /*
> * TTBCR
> * We must write this before the TTBRs, since it determines the
> * access behaviour of some fields (in particular, ASID[15:8]).
> */
> - if (stage1) {
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> - reg = pgtbl_cfg->arm_v7s_cfg.tcr;
> - reg2 = 0;
> - } else {
> - reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
> - reg2 |= TTBCR2_SEP_UPSTREAM;
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> - reg2 |= TTBCR2_AS;
> - }
> - if (smmu->version > ARM_SMMU_V1)
> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
> - } else {
> - reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
> - }
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
> + if (stage1 && smmu->version > ARM_SMMU_V1)
> + writel_relaxed(cb->tcr[1], cb_base + ARM_SMMU_CB_TTBCR2);
> + writel_relaxed(cb->tcr[0], cb_base + ARM_SMMU_CB_TTBCR);
>
> /* TTBRs */
> - if (stage1) {
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0);
> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1);
> - writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
> - } else {
> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR1);
> - }
> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> + writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
> + writel_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
> + writel_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
> } else {
> - reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
> + writeq_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
> + writeq_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
> }
>
> /* MAIRs (stage-1 only) */
> if (stage1) {
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> - reg = pgtbl_cfg->arm_v7s_cfg.prrr;
> - reg2 = pgtbl_cfg->arm_v7s_cfg.nmrr;
> - } else {
> - reg = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
> - }
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_S1_MAIR0);
> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_S1_MAIR1);
> + writel_relaxed(cb->mair[0], cb_base + ARM_SMMU_CB_S1_MAIR0);
> + writel_relaxed(cb->mair[1], cb_base + ARM_SMMU_CB_S1_MAIR1);
> }
>
> /* SCTLR */
> - reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
> - if (stage1)
> - reg |= SCTLR_S1_ASIDPNE;
> -#ifdef __BIG_ENDIAN
> - reg |= SCTLR_E;
> -#endif
> + if (!cb->cfg) {
> + reg = 0;
> + } else {
> + reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
> + if (stage1)
> + reg |= SCTLR_S1_ASIDPNE;
> + if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> + reg |= SCTLR_E;
> + }
> writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
> }
>
> @@ -1035,6 +1066,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>
> /* Initialise the context bank with our page table cfg */
> arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
>
> /*
> * Request context fault interrupt. Do this last to avoid the
> @@ -1067,7 +1099,6 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> struct arm_smmu_device *smmu = smmu_domain->smmu;
> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> - void __iomem *cb_base;
> int irq;
>
> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
> @@ -1077,8 +1108,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
> * Disable the context bank and free the page tables before freeing
> * it.
> */
> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
> - writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
> + smmu->cbs[cfg->cbndx] = (struct arm_smmu_cb){0};
> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
>
> if (cfg->irptndx != INVALID_IRPTNDX) {
> irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx];
> @@ -1722,7 +1753,6 @@ static struct iommu_ops arm_smmu_ops = {
> static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> {
> void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> - void __iomem *cb_base;
> int i;
> u32 reg, major;
>
> @@ -1758,8 +1788,9 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>
> /* Make sure all context banks are disabled and clear CB_FSR */
> for (i = 0; i < smmu->num_context_banks; ++i) {
> - cb_base = ARM_SMMU_CB(smmu, i);
> - writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
> + void __iomem *cb_base = ARM_SMMU_CB(smmu, i);
> +
> + arm_smmu_write_context_bank(smmu, i);
> writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
> /*
> * Disable MMU-500's not-particularly-beneficial next-page
> @@ -1964,6 +1995,10 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
> smmu->cavium_id_base -= smmu->num_context_banks;
> dev_notice(smmu->dev, "\tenabling workaround for Cavium erratum 27704\n");
> }
> + smmu->cbs = devm_kcalloc(smmu->dev, smmu->num_context_banks,
> + sizeof(*smmu->cbs), GFP_KERNEL);
> + if (!smmu->cbs)
> + return -ENOMEM;
>
> /* ID2 */
> id = readl_relaxed(gr0_base + ARM_SMMU_GR0_ID2);
>
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] iommu/arm-smmu: Track context bank state
@ 2017-07-19 2:40 ` Sricharan R
0 siblings, 0 replies; 18+ messages in thread
From: Sricharan R @ 2017-07-19 2:40 UTC (permalink / raw)
To: linux-arm-kernel
Hi Robin,
On 7/18/2017 6:14 PM, Robin Murphy wrote:
> Echoing what we do for Stream Map Entries, maintain a software shadow
> state for context bank configuration. With this in place, we are mere
> moments away from blissfully easy suspend/resume support.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>
> Since the runtime PM discussion has come back again, I figured it was
> probably about time to finish off my plan for system PM. Lightly tested
> on Juno (MMU-401) with hibernation.
>
> Robin.
>
> drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++++------------------
> 1 file changed, 97 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index bc89b4d6c043..86897b7b81d8 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -338,6 +338,13 @@ struct arm_smmu_smr {
> bool valid;
> };
>
> +struct arm_smmu_cb {
> + u64 ttbr[2];
> + u32 tcr[2];
> + u32 mair[2];
> + struct arm_smmu_cfg *cfg;
> +};
> +
When i was trying this sometime back [1], was
saving and using the pgtbl->cfg and domain->cfg for
restoring the context. But this looks correct to make
a actual shadow once and use it later all the time.
Will also try some testing today.
Reviewed-by: sricharan at codeaurora.org
[1] https://patchwork.kernel.org/patch/9389717/
Regards,
Sricharan
> struct arm_smmu_master_cfg {
> struct arm_smmu_device *smmu;
> s16 smendx[];
> @@ -380,6 +387,7 @@ struct arm_smmu_device {
> u32 num_context_banks;
> u32 num_s2_context_banks;
> DECLARE_BITMAP(context_map, ARM_SMMU_MAX_CBS);
> + struct arm_smmu_cb *cbs;
> atomic_t irptndx;
>
> u32 num_mapping_groups;
> @@ -768,17 +776,69 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
> static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> struct io_pgtable_cfg *pgtbl_cfg)
> {
> - u32 reg, reg2;
> - u64 reg64;
> - bool stage1;
> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> - struct arm_smmu_device *smmu = smmu_domain->smmu;
> + struct arm_smmu_cb *cb = &smmu_domain->smmu->cbs[cfg->cbndx];
> + bool stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
> +
> + cb->cfg = cfg;
> +
> + /* TTBCR */
> + if (stage1) {
> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> + cb->tcr[0] = pgtbl_cfg->arm_v7s_cfg.tcr;
> + } else {
> + cb->tcr[0] = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
> + cb->tcr[1] = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
> + cb->tcr[1] |= TTBCR2_SEP_UPSTREAM;
> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> + cb->tcr[1] |= TTBCR2_AS;
> + }
> + } else {
> + cb->tcr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
> + }
> +
> + /* TTBRs */
> + if (stage1) {
> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> + cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
> + cb->ttbr[1] = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
> + } else {
> + cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
> + cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
> + }
> + } else {
> + cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
> + }
> +
> + /* MAIRs (stage-1 only) */
> + if (stage1) {
> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> + cb->mair[0] = pgtbl_cfg->arm_v7s_cfg.prrr;
> + cb->mair[1] = pgtbl_cfg->arm_v7s_cfg.nmrr;
> + } else {
> + cb->mair[0] = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
> + cb->mair[1] = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
> + }
> + }
> +}
> +
> +static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
> +{
> + u32 reg;
> + bool stage1;
> + struct arm_smmu_cb *cb = &smmu->cbs[idx];
> + struct arm_smmu_cfg *cfg = cb->cfg;
> + struct arm_smmu_cfg default_cfg = {0};
> void __iomem *cb_base, *gr1_base;
>
> + if (!cfg)
> + cfg = &default_cfg;
> +
> gr1_base = ARM_SMMU_GR1(smmu);
> stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
> + cb_base = ARM_SMMU_CB(smmu, idx);
>
> + /* CBA2R */
> if (smmu->version > ARM_SMMU_V1) {
> if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> reg = CBA2R_RW64_64BIT;
> @@ -788,7 +848,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> if (smmu->features & ARM_SMMU_FEAT_VMID16)
> reg |= cfg->vmid << CBA2R_VMID_SHIFT;
>
> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(idx));
> }
>
> /* CBAR */
> @@ -807,72 +867,43 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> /* 8-bit VMIDs live in CBAR */
> reg |= cfg->vmid << CBAR_VMID_SHIFT;
> }
> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(idx));
>
> /*
> * TTBCR
> * We must write this before the TTBRs, since it determines the
> * access behaviour of some fields (in particular, ASID[15:8]).
> */
> - if (stage1) {
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> - reg = pgtbl_cfg->arm_v7s_cfg.tcr;
> - reg2 = 0;
> - } else {
> - reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
> - reg2 |= TTBCR2_SEP_UPSTREAM;
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> - reg2 |= TTBCR2_AS;
> - }
> - if (smmu->version > ARM_SMMU_V1)
> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
> - } else {
> - reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
> - }
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
> + if (stage1 && smmu->version > ARM_SMMU_V1)
> + writel_relaxed(cb->tcr[1], cb_base + ARM_SMMU_CB_TTBCR2);
> + writel_relaxed(cb->tcr[0], cb_base + ARM_SMMU_CB_TTBCR);
>
> /* TTBRs */
> - if (stage1) {
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0);
> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1);
> - writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
> - } else {
> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR1);
> - }
> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> + writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
> + writel_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
> + writel_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
> } else {
> - reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
> + writeq_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
> + writeq_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
> }
>
> /* MAIRs (stage-1 only) */
> if (stage1) {
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> - reg = pgtbl_cfg->arm_v7s_cfg.prrr;
> - reg2 = pgtbl_cfg->arm_v7s_cfg.nmrr;
> - } else {
> - reg = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
> - }
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_S1_MAIR0);
> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_S1_MAIR1);
> + writel_relaxed(cb->mair[0], cb_base + ARM_SMMU_CB_S1_MAIR0);
> + writel_relaxed(cb->mair[1], cb_base + ARM_SMMU_CB_S1_MAIR1);
> }
>
> /* SCTLR */
> - reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
> - if (stage1)
> - reg |= SCTLR_S1_ASIDPNE;
> -#ifdef __BIG_ENDIAN
> - reg |= SCTLR_E;
> -#endif
> + if (!cb->cfg) {
> + reg = 0;
> + } else {
> + reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
> + if (stage1)
> + reg |= SCTLR_S1_ASIDPNE;
> + if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> + reg |= SCTLR_E;
> + }
> writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
> }
>
> @@ -1035,6 +1066,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>
> /* Initialise the context bank with our page table cfg */
> arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
>
> /*
> * Request context fault interrupt. Do this last to avoid the
> @@ -1067,7 +1099,6 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> struct arm_smmu_device *smmu = smmu_domain->smmu;
> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> - void __iomem *cb_base;
> int irq;
>
> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
> @@ -1077,8 +1108,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
> * Disable the context bank and free the page tables before freeing
> * it.
> */
> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
> - writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
> + smmu->cbs[cfg->cbndx] = (struct arm_smmu_cb){0};
> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
>
> if (cfg->irptndx != INVALID_IRPTNDX) {
> irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx];
> @@ -1722,7 +1753,6 @@ static struct iommu_ops arm_smmu_ops = {
> static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> {
> void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> - void __iomem *cb_base;
> int i;
> u32 reg, major;
>
> @@ -1758,8 +1788,9 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>
> /* Make sure all context banks are disabled and clear CB_FSR */
> for (i = 0; i < smmu->num_context_banks; ++i) {
> - cb_base = ARM_SMMU_CB(smmu, i);
> - writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
> + void __iomem *cb_base = ARM_SMMU_CB(smmu, i);
> +
> + arm_smmu_write_context_bank(smmu, i);
> writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
> /*
> * Disable MMU-500's not-particularly-beneficial next-page
> @@ -1964,6 +1995,10 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
> smmu->cavium_id_base -= smmu->num_context_banks;
> dev_notice(smmu->dev, "\tenabling workaround for Cavium erratum 27704\n");
> }
> + smmu->cbs = devm_kcalloc(smmu->dev, smmu->num_context_banks,
> + sizeof(*smmu->cbs), GFP_KERNEL);
> + if (!smmu->cbs)
> + return -ENOMEM;
>
> /* ID2 */
> id = readl_relaxed(gr0_base + ARM_SMMU_GR0_ID2);
>
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] iommu/arm-smmu: Track context bank state
2017-07-19 2:40 ` Sricharan R
@ 2017-07-19 10:17 ` Robin Murphy
-1 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2017-07-19 10:17 UTC (permalink / raw)
To: Sricharan R, will.deacon-5wv7dgnIgG8, joro-zLv9SwRftAIdnm+yROfE0A
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 19/07/17 03:40, Sricharan R wrote:
> Hi Robin,
>
> On 7/18/2017 6:14 PM, Robin Murphy wrote:
>> Echoing what we do for Stream Map Entries, maintain a software shadow
>> state for context bank configuration. With this in place, we are mere
>> moments away from blissfully easy suspend/resume support.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>
>> Since the runtime PM discussion has come back again, I figured it was
>> probably about time to finish off my plan for system PM. Lightly tested
>> on Juno (MMU-401) with hibernation.
>>
>> Robin.
>>
>> drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++++------------------
>> 1 file changed, 97 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index bc89b4d6c043..86897b7b81d8 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -338,6 +338,13 @@ struct arm_smmu_smr {
>> bool valid;
>> };
>>
>> +struct arm_smmu_cb {
>> + u64 ttbr[2];
>> + u32 tcr[2];
>> + u32 mair[2];
>> + struct arm_smmu_cfg *cfg;
>> +};
>> +
>
> When i was trying this sometime back [1], was
> saving and using the pgtbl->cfg and domain->cfg for
> restoring the context. But this looks correct to make
> a actual shadow once and use it later all the time.
Yeah, I did have a go at using the io_pgtable_cfg, but given the
variable format of the registers involved it turned out simpler to
shadow the raw register state directly.
> Will also try some testing today.
>
> Reviewed-by: sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org
Thanks!
Robin.
> [1] https://patchwork.kernel.org/patch/9389717/
>
> Regards,
> Sricharan
>
>
>> struct arm_smmu_master_cfg {
>> struct arm_smmu_device *smmu;
>> s16 smendx[];
>> @@ -380,6 +387,7 @@ struct arm_smmu_device {
>> u32 num_context_banks;
>> u32 num_s2_context_banks;
>> DECLARE_BITMAP(context_map, ARM_SMMU_MAX_CBS);
>> + struct arm_smmu_cb *cbs;
>> atomic_t irptndx;
>>
>> u32 num_mapping_groups;
>> @@ -768,17 +776,69 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
>> static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>> struct io_pgtable_cfg *pgtbl_cfg)
>> {
>> - u32 reg, reg2;
>> - u64 reg64;
>> - bool stage1;
>> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>> - struct arm_smmu_device *smmu = smmu_domain->smmu;
>> + struct arm_smmu_cb *cb = &smmu_domain->smmu->cbs[cfg->cbndx];
>> + bool stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
>> +
>> + cb->cfg = cfg;
>> +
>> + /* TTBCR */
>> + if (stage1) {
>> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> + cb->tcr[0] = pgtbl_cfg->arm_v7s_cfg.tcr;
>> + } else {
>> + cb->tcr[0] = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
>> + cb->tcr[1] = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
>> + cb->tcr[1] |= TTBCR2_SEP_UPSTREAM;
>> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>> + cb->tcr[1] |= TTBCR2_AS;
>> + }
>> + } else {
>> + cb->tcr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
>> + }
>> +
>> + /* TTBRs */
>> + if (stage1) {
>> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> + cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
>> + cb->ttbr[1] = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
>> + } else {
>> + cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
>> + cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
>> + }
>> + } else {
>> + cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
>> + }
>> +
>> + /* MAIRs (stage-1 only) */
>> + if (stage1) {
>> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> + cb->mair[0] = pgtbl_cfg->arm_v7s_cfg.prrr;
>> + cb->mair[1] = pgtbl_cfg->arm_v7s_cfg.nmrr;
>> + } else {
>> + cb->mair[0] = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
>> + cb->mair[1] = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
>> + }
>> + }
>> +}
>> +
>> +static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
>> +{
>> + u32 reg;
>> + bool stage1;
>> + struct arm_smmu_cb *cb = &smmu->cbs[idx];
>> + struct arm_smmu_cfg *cfg = cb->cfg;
>> + struct arm_smmu_cfg default_cfg = {0};
>> void __iomem *cb_base, *gr1_base;
>>
>> + if (!cfg)
>> + cfg = &default_cfg;
>> +
>> gr1_base = ARM_SMMU_GR1(smmu);
>> stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
>> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
>> + cb_base = ARM_SMMU_CB(smmu, idx);
>>
>> + /* CBA2R */
>> if (smmu->version > ARM_SMMU_V1) {
>> if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>> reg = CBA2R_RW64_64BIT;
>> @@ -788,7 +848,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>> if (smmu->features & ARM_SMMU_FEAT_VMID16)
>> reg |= cfg->vmid << CBA2R_VMID_SHIFT;
>>
>> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
>> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(idx));
>> }
>>
>> /* CBAR */
>> @@ -807,72 +867,43 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>> /* 8-bit VMIDs live in CBAR */
>> reg |= cfg->vmid << CBAR_VMID_SHIFT;
>> }
>> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
>> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(idx));
>>
>> /*
>> * TTBCR
>> * We must write this before the TTBRs, since it determines the
>> * access behaviour of some fields (in particular, ASID[15:8]).
>> */
>> - if (stage1) {
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> - reg = pgtbl_cfg->arm_v7s_cfg.tcr;
>> - reg2 = 0;
>> - } else {
>> - reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
>> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
>> - reg2 |= TTBCR2_SEP_UPSTREAM;
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>> - reg2 |= TTBCR2_AS;
>> - }
>> - if (smmu->version > ARM_SMMU_V1)
>> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
>> - } else {
>> - reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
>> - }
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
>> + if (stage1 && smmu->version > ARM_SMMU_V1)
>> + writel_relaxed(cb->tcr[1], cb_base + ARM_SMMU_CB_TTBCR2);
>> + writel_relaxed(cb->tcr[0], cb_base + ARM_SMMU_CB_TTBCR);
>>
>> /* TTBRs */
>> - if (stage1) {
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0);
>> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1);
>> - writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
>> - } else {
>> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
>> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
>> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
>> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
>> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
>> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR1);
>> - }
>> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> + writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
>> + writel_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
>> + writel_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
>> } else {
>> - reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
>> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
>> + writeq_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
>> + writeq_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
>> }
>>
>> /* MAIRs (stage-1 only) */
>> if (stage1) {
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> - reg = pgtbl_cfg->arm_v7s_cfg.prrr;
>> - reg2 = pgtbl_cfg->arm_v7s_cfg.nmrr;
>> - } else {
>> - reg = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
>> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
>> - }
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_S1_MAIR0);
>> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_S1_MAIR1);
>> + writel_relaxed(cb->mair[0], cb_base + ARM_SMMU_CB_S1_MAIR0);
>> + writel_relaxed(cb->mair[1], cb_base + ARM_SMMU_CB_S1_MAIR1);
>> }
>>
>> /* SCTLR */
>> - reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
>> - if (stage1)
>> - reg |= SCTLR_S1_ASIDPNE;
>> -#ifdef __BIG_ENDIAN
>> - reg |= SCTLR_E;
>> -#endif
>> + if (!cb->cfg) {
>> + reg = 0;
>> + } else {
>> + reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
>> + if (stage1)
>> + reg |= SCTLR_S1_ASIDPNE;
>> + if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>> + reg |= SCTLR_E;
>> + }
>> writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
>> }
>>
>> @@ -1035,6 +1066,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>>
>> /* Initialise the context bank with our page table cfg */
>> arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
>> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
>>
>> /*
>> * Request context fault interrupt. Do this last to avoid the
>> @@ -1067,7 +1099,6 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> struct arm_smmu_device *smmu = smmu_domain->smmu;
>> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>> - void __iomem *cb_base;
>> int irq;
>>
>> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
>> @@ -1077,8 +1108,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>> * Disable the context bank and free the page tables before freeing
>> * it.
>> */
>> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
>> - writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
>> + smmu->cbs[cfg->cbndx] = (struct arm_smmu_cb){0};
>> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
>>
>> if (cfg->irptndx != INVALID_IRPTNDX) {
>> irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx];
>> @@ -1722,7 +1753,6 @@ static struct iommu_ops arm_smmu_ops = {
>> static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>> {
>> void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
>> - void __iomem *cb_base;
>> int i;
>> u32 reg, major;
>>
>> @@ -1758,8 +1788,9 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>>
>> /* Make sure all context banks are disabled and clear CB_FSR */
>> for (i = 0; i < smmu->num_context_banks; ++i) {
>> - cb_base = ARM_SMMU_CB(smmu, i);
>> - writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
>> + void __iomem *cb_base = ARM_SMMU_CB(smmu, i);
>> +
>> + arm_smmu_write_context_bank(smmu, i);
>> writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
>> /*
>> * Disable MMU-500's not-particularly-beneficial next-page
>> @@ -1964,6 +1995,10 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>> smmu->cavium_id_base -= smmu->num_context_banks;
>> dev_notice(smmu->dev, "\tenabling workaround for Cavium erratum 27704\n");
>> }
>> + smmu->cbs = devm_kcalloc(smmu->dev, smmu->num_context_banks,
>> + sizeof(*smmu->cbs), GFP_KERNEL);
>> + if (!smmu->cbs)
>> + return -ENOMEM;
>>
>> /* ID2 */
>> id = readl_relaxed(gr0_base + ARM_SMMU_GR0_ID2);
>>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] iommu/arm-smmu: Track context bank state
@ 2017-07-19 10:17 ` Robin Murphy
0 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2017-07-19 10:17 UTC (permalink / raw)
To: linux-arm-kernel
On 19/07/17 03:40, Sricharan R wrote:
> Hi Robin,
>
> On 7/18/2017 6:14 PM, Robin Murphy wrote:
>> Echoing what we do for Stream Map Entries, maintain a software shadow
>> state for context bank configuration. With this in place, we are mere
>> moments away from blissfully easy suspend/resume support.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> Since the runtime PM discussion has come back again, I figured it was
>> probably about time to finish off my plan for system PM. Lightly tested
>> on Juno (MMU-401) with hibernation.
>>
>> Robin.
>>
>> drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++++------------------
>> 1 file changed, 97 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index bc89b4d6c043..86897b7b81d8 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -338,6 +338,13 @@ struct arm_smmu_smr {
>> bool valid;
>> };
>>
>> +struct arm_smmu_cb {
>> + u64 ttbr[2];
>> + u32 tcr[2];
>> + u32 mair[2];
>> + struct arm_smmu_cfg *cfg;
>> +};
>> +
>
> When i was trying this sometime back [1], was
> saving and using the pgtbl->cfg and domain->cfg for
> restoring the context. But this looks correct to make
> a actual shadow once and use it later all the time.
Yeah, I did have a go at using the io_pgtable_cfg, but given the
variable format of the registers involved it turned out simpler to
shadow the raw register state directly.
> Will also try some testing today.
>
> Reviewed-by: sricharan at codeaurora.org
Thanks!
Robin.
> [1] https://patchwork.kernel.org/patch/9389717/
>
> Regards,
> Sricharan
>
>
>> struct arm_smmu_master_cfg {
>> struct arm_smmu_device *smmu;
>> s16 smendx[];
>> @@ -380,6 +387,7 @@ struct arm_smmu_device {
>> u32 num_context_banks;
>> u32 num_s2_context_banks;
>> DECLARE_BITMAP(context_map, ARM_SMMU_MAX_CBS);
>> + struct arm_smmu_cb *cbs;
>> atomic_t irptndx;
>>
>> u32 num_mapping_groups;
>> @@ -768,17 +776,69 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
>> static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>> struct io_pgtable_cfg *pgtbl_cfg)
>> {
>> - u32 reg, reg2;
>> - u64 reg64;
>> - bool stage1;
>> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>> - struct arm_smmu_device *smmu = smmu_domain->smmu;
>> + struct arm_smmu_cb *cb = &smmu_domain->smmu->cbs[cfg->cbndx];
>> + bool stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
>> +
>> + cb->cfg = cfg;
>> +
>> + /* TTBCR */
>> + if (stage1) {
>> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> + cb->tcr[0] = pgtbl_cfg->arm_v7s_cfg.tcr;
>> + } else {
>> + cb->tcr[0] = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
>> + cb->tcr[1] = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
>> + cb->tcr[1] |= TTBCR2_SEP_UPSTREAM;
>> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>> + cb->tcr[1] |= TTBCR2_AS;
>> + }
>> + } else {
>> + cb->tcr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
>> + }
>> +
>> + /* TTBRs */
>> + if (stage1) {
>> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> + cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
>> + cb->ttbr[1] = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
>> + } else {
>> + cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
>> + cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
>> + }
>> + } else {
>> + cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
>> + }
>> +
>> + /* MAIRs (stage-1 only) */
>> + if (stage1) {
>> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> + cb->mair[0] = pgtbl_cfg->arm_v7s_cfg.prrr;
>> + cb->mair[1] = pgtbl_cfg->arm_v7s_cfg.nmrr;
>> + } else {
>> + cb->mair[0] = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
>> + cb->mair[1] = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
>> + }
>> + }
>> +}
>> +
>> +static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
>> +{
>> + u32 reg;
>> + bool stage1;
>> + struct arm_smmu_cb *cb = &smmu->cbs[idx];
>> + struct arm_smmu_cfg *cfg = cb->cfg;
>> + struct arm_smmu_cfg default_cfg = {0};
>> void __iomem *cb_base, *gr1_base;
>>
>> + if (!cfg)
>> + cfg = &default_cfg;
>> +
>> gr1_base = ARM_SMMU_GR1(smmu);
>> stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
>> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
>> + cb_base = ARM_SMMU_CB(smmu, idx);
>>
>> + /* CBA2R */
>> if (smmu->version > ARM_SMMU_V1) {
>> if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>> reg = CBA2R_RW64_64BIT;
>> @@ -788,7 +848,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>> if (smmu->features & ARM_SMMU_FEAT_VMID16)
>> reg |= cfg->vmid << CBA2R_VMID_SHIFT;
>>
>> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
>> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(idx));
>> }
>>
>> /* CBAR */
>> @@ -807,72 +867,43 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>> /* 8-bit VMIDs live in CBAR */
>> reg |= cfg->vmid << CBAR_VMID_SHIFT;
>> }
>> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
>> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(idx));
>>
>> /*
>> * TTBCR
>> * We must write this before the TTBRs, since it determines the
>> * access behaviour of some fields (in particular, ASID[15:8]).
>> */
>> - if (stage1) {
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> - reg = pgtbl_cfg->arm_v7s_cfg.tcr;
>> - reg2 = 0;
>> - } else {
>> - reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
>> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
>> - reg2 |= TTBCR2_SEP_UPSTREAM;
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>> - reg2 |= TTBCR2_AS;
>> - }
>> - if (smmu->version > ARM_SMMU_V1)
>> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
>> - } else {
>> - reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
>> - }
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
>> + if (stage1 && smmu->version > ARM_SMMU_V1)
>> + writel_relaxed(cb->tcr[1], cb_base + ARM_SMMU_CB_TTBCR2);
>> + writel_relaxed(cb->tcr[0], cb_base + ARM_SMMU_CB_TTBCR);
>>
>> /* TTBRs */
>> - if (stage1) {
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0);
>> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1);
>> - writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
>> - } else {
>> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
>> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
>> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
>> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
>> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
>> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR1);
>> - }
>> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> + writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
>> + writel_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
>> + writel_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
>> } else {
>> - reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
>> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
>> + writeq_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
>> + writeq_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
>> }
>>
>> /* MAIRs (stage-1 only) */
>> if (stage1) {
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> - reg = pgtbl_cfg->arm_v7s_cfg.prrr;
>> - reg2 = pgtbl_cfg->arm_v7s_cfg.nmrr;
>> - } else {
>> - reg = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
>> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
>> - }
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_S1_MAIR0);
>> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_S1_MAIR1);
>> + writel_relaxed(cb->mair[0], cb_base + ARM_SMMU_CB_S1_MAIR0);
>> + writel_relaxed(cb->mair[1], cb_base + ARM_SMMU_CB_S1_MAIR1);
>> }
>>
>> /* SCTLR */
>> - reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
>> - if (stage1)
>> - reg |= SCTLR_S1_ASIDPNE;
>> -#ifdef __BIG_ENDIAN
>> - reg |= SCTLR_E;
>> -#endif
>> + if (!cb->cfg) {
>> + reg = 0;
>> + } else {
>> + reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
>> + if (stage1)
>> + reg |= SCTLR_S1_ASIDPNE;
>> + if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>> + reg |= SCTLR_E;
>> + }
>> writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
>> }
>>
>> @@ -1035,6 +1066,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>>
>> /* Initialise the context bank with our page table cfg */
>> arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
>> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
>>
>> /*
>> * Request context fault interrupt. Do this last to avoid the
>> @@ -1067,7 +1099,6 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> struct arm_smmu_device *smmu = smmu_domain->smmu;
>> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>> - void __iomem *cb_base;
>> int irq;
>>
>> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
>> @@ -1077,8 +1108,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>> * Disable the context bank and free the page tables before freeing
>> * it.
>> */
>> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
>> - writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
>> + smmu->cbs[cfg->cbndx] = (struct arm_smmu_cb){0};
>> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
>>
>> if (cfg->irptndx != INVALID_IRPTNDX) {
>> irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx];
>> @@ -1722,7 +1753,6 @@ static struct iommu_ops arm_smmu_ops = {
>> static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>> {
>> void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
>> - void __iomem *cb_base;
>> int i;
>> u32 reg, major;
>>
>> @@ -1758,8 +1788,9 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>>
>> /* Make sure all context banks are disabled and clear CB_FSR */
>> for (i = 0; i < smmu->num_context_banks; ++i) {
>> - cb_base = ARM_SMMU_CB(smmu, i);
>> - writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
>> + void __iomem *cb_base = ARM_SMMU_CB(smmu, i);
>> +
>> + arm_smmu_write_context_bank(smmu, i);
>> writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
>> /*
>> * Disable MMU-500's not-particularly-beneficial next-page
>> @@ -1964,6 +1995,10 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>> smmu->cavium_id_base -= smmu->num_context_banks;
>> dev_notice(smmu->dev, "\tenabling workaround for Cavium erratum 27704\n");
>> }
>> + smmu->cbs = devm_kcalloc(smmu->dev, smmu->num_context_banks,
>> + sizeof(*smmu->cbs), GFP_KERNEL);
>> + if (!smmu->cbs)
>> + return -ENOMEM;
>>
>> /* ID2 */
>> id = readl_relaxed(gr0_base + ARM_SMMU_GR0_ID2);
>>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] iommu/arm-smmu: Track context bank state
2017-07-18 12:44 ` Robin Murphy
@ 2017-08-08 11:11 ` Will Deacon
-1 siblings, 0 replies; 18+ messages in thread
From: Will Deacon @ 2017-08-08 11:11 UTC (permalink / raw)
To: Robin Murphy
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Hi Robin,
I like the idea, but I think there are a few minor issues with the patch.
Comments below.
On Tue, Jul 18, 2017 at 01:44:41PM +0100, Robin Murphy wrote:
> Echoing what we do for Stream Map Entries, maintain a software shadow
> state for context bank configuration. With this in place, we are mere
> moments away from blissfully easy suspend/resume support.
>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>
> Since the runtime PM discussion has come back again, I figured it was
> probably about time to finish off my plan for system PM. Lightly tested
> on Juno (MMU-401) with hibernation.
>
> Robin.
>
> drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++++------------------
> 1 file changed, 97 insertions(+), 62 deletions(-)
[...]
> +static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
> +{
> + u32 reg;
> + bool stage1;
> + struct arm_smmu_cb *cb = &smmu->cbs[idx];
> + struct arm_smmu_cfg *cfg = cb->cfg;
> + struct arm_smmu_cfg default_cfg = {0};
> void __iomem *cb_base, *gr1_base;
>
> + if (!cfg)
> + cfg = &default_cfg;
> +
> gr1_base = ARM_SMMU_GR1(smmu);
> stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
> + cb_base = ARM_SMMU_CB(smmu, idx);
>
> + /* CBA2R */
> if (smmu->version > ARM_SMMU_V1) {
> if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> reg = CBA2R_RW64_64BIT;
> @@ -788,7 +848,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> if (smmu->features & ARM_SMMU_FEAT_VMID16)
> reg |= cfg->vmid << CBA2R_VMID_SHIFT;
>
> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(idx));
> }
>
> /* CBAR */
> @@ -807,72 +867,43 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> /* 8-bit VMIDs live in CBAR */
> reg |= cfg->vmid << CBAR_VMID_SHIFT;
> }
> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(idx));
>
> /*
> * TTBCR
> * We must write this before the TTBRs, since it determines the
> * access behaviour of some fields (in particular, ASID[15:8]).
> */
> - if (stage1) {
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> - reg = pgtbl_cfg->arm_v7s_cfg.tcr;
> - reg2 = 0;
> - } else {
> - reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
> - reg2 |= TTBCR2_SEP_UPSTREAM;
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> - reg2 |= TTBCR2_AS;
> - }
> - if (smmu->version > ARM_SMMU_V1)
> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
> - } else {
> - reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
> - }
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
> + if (stage1 && smmu->version > ARM_SMMU_V1)
> + writel_relaxed(cb->tcr[1], cb_base + ARM_SMMU_CB_TTBCR2);
> + writel_relaxed(cb->tcr[0], cb_base + ARM_SMMU_CB_TTBCR);
>
> /* TTBRs */
> - if (stage1) {
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0);
> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1);
> - writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
> - } else {
> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR1);
> - }
> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> + writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
> + writel_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
> + writel_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
> } else {
> - reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
> + writeq_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
> + writeq_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
This doesn't look right to me. For the LPAE S1 case, we don't set the ASID
afaict, and for the S2 case we write to a RESERVED register (since we only
have one TTBR).
> }
>
> /* MAIRs (stage-1 only) */
> if (stage1) {
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> - reg = pgtbl_cfg->arm_v7s_cfg.prrr;
> - reg2 = pgtbl_cfg->arm_v7s_cfg.nmrr;
> - } else {
> - reg = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
> - }
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_S1_MAIR0);
> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_S1_MAIR1);
> + writel_relaxed(cb->mair[0], cb_base + ARM_SMMU_CB_S1_MAIR0);
> + writel_relaxed(cb->mair[1], cb_base + ARM_SMMU_CB_S1_MAIR1);
> }
>
> /* SCTLR */
> - reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
> - if (stage1)
> - reg |= SCTLR_S1_ASIDPNE;
> -#ifdef __BIG_ENDIAN
> - reg |= SCTLR_E;
> -#endif
> + if (!cb->cfg) {
> + reg = 0;
I think this might be too late. See below.
> + } else {
> + reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
> + if (stage1)
> + reg |= SCTLR_S1_ASIDPNE;
> + if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> + reg |= SCTLR_E;
> + }
> writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
> }
>
> @@ -1035,6 +1066,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>
> /* Initialise the context bank with our page table cfg */
> arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
>
> /*
> * Request context fault interrupt. Do this last to avoid the
> @@ -1067,7 +1099,6 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> struct arm_smmu_device *smmu = smmu_domain->smmu;
> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> - void __iomem *cb_base;
> int irq;
>
> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
> @@ -1077,8 +1108,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
> * Disable the context bank and free the page tables before freeing
> * it.
> */
> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
> - writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
> + smmu->cbs[cfg->cbndx] = (struct arm_smmu_cb){0};
> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
So here, we end up passing a zeroed structure to
arm_smmu_write_context_bank, but afaict that will write to the TTBR and TCR
before SCTLR, which worries me. Couldn't we get whacky speculative table
walks through physical address 0 with ASID 0 before we kill the SCTLR?
If the only time we pass something with a NULL cfg is on the destroy path,
perhaps just do:
if (!cfg) {
writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
return;
}
instead?
Will
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] iommu/arm-smmu: Track context bank state
@ 2017-08-08 11:11 ` Will Deacon
0 siblings, 0 replies; 18+ messages in thread
From: Will Deacon @ 2017-08-08 11:11 UTC (permalink / raw)
To: linux-arm-kernel
Hi Robin,
I like the idea, but I think there are a few minor issues with the patch.
Comments below.
On Tue, Jul 18, 2017 at 01:44:41PM +0100, Robin Murphy wrote:
> Echoing what we do for Stream Map Entries, maintain a software shadow
> state for context bank configuration. With this in place, we are mere
> moments away from blissfully easy suspend/resume support.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>
> Since the runtime PM discussion has come back again, I figured it was
> probably about time to finish off my plan for system PM. Lightly tested
> on Juno (MMU-401) with hibernation.
>
> Robin.
>
> drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++++------------------
> 1 file changed, 97 insertions(+), 62 deletions(-)
[...]
> +static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
> +{
> + u32 reg;
> + bool stage1;
> + struct arm_smmu_cb *cb = &smmu->cbs[idx];
> + struct arm_smmu_cfg *cfg = cb->cfg;
> + struct arm_smmu_cfg default_cfg = {0};
> void __iomem *cb_base, *gr1_base;
>
> + if (!cfg)
> + cfg = &default_cfg;
> +
> gr1_base = ARM_SMMU_GR1(smmu);
> stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
> + cb_base = ARM_SMMU_CB(smmu, idx);
>
> + /* CBA2R */
> if (smmu->version > ARM_SMMU_V1) {
> if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> reg = CBA2R_RW64_64BIT;
> @@ -788,7 +848,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> if (smmu->features & ARM_SMMU_FEAT_VMID16)
> reg |= cfg->vmid << CBA2R_VMID_SHIFT;
>
> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(idx));
> }
>
> /* CBAR */
> @@ -807,72 +867,43 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> /* 8-bit VMIDs live in CBAR */
> reg |= cfg->vmid << CBAR_VMID_SHIFT;
> }
> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(idx));
>
> /*
> * TTBCR
> * We must write this before the TTBRs, since it determines the
> * access behaviour of some fields (in particular, ASID[15:8]).
> */
> - if (stage1) {
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> - reg = pgtbl_cfg->arm_v7s_cfg.tcr;
> - reg2 = 0;
> - } else {
> - reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
> - reg2 |= TTBCR2_SEP_UPSTREAM;
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> - reg2 |= TTBCR2_AS;
> - }
> - if (smmu->version > ARM_SMMU_V1)
> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
> - } else {
> - reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
> - }
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
> + if (stage1 && smmu->version > ARM_SMMU_V1)
> + writel_relaxed(cb->tcr[1], cb_base + ARM_SMMU_CB_TTBCR2);
> + writel_relaxed(cb->tcr[0], cb_base + ARM_SMMU_CB_TTBCR);
>
> /* TTBRs */
> - if (stage1) {
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0);
> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1);
> - writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
> - } else {
> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR1);
> - }
> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> + writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
> + writel_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
> + writel_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
> } else {
> - reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
> + writeq_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
> + writeq_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
This doesn't look right to me. For the LPAE S1 case, we don't set the ASID
afaict, and for the S2 case we write to a RESERVED register (since we only
have one TTBR).
> }
>
> /* MAIRs (stage-1 only) */
> if (stage1) {
> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> - reg = pgtbl_cfg->arm_v7s_cfg.prrr;
> - reg2 = pgtbl_cfg->arm_v7s_cfg.nmrr;
> - } else {
> - reg = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
> - }
> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_S1_MAIR0);
> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_S1_MAIR1);
> + writel_relaxed(cb->mair[0], cb_base + ARM_SMMU_CB_S1_MAIR0);
> + writel_relaxed(cb->mair[1], cb_base + ARM_SMMU_CB_S1_MAIR1);
> }
>
> /* SCTLR */
> - reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
> - if (stage1)
> - reg |= SCTLR_S1_ASIDPNE;
> -#ifdef __BIG_ENDIAN
> - reg |= SCTLR_E;
> -#endif
> + if (!cb->cfg) {
> + reg = 0;
I think this might be too late. See below.
> + } else {
> + reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
> + if (stage1)
> + reg |= SCTLR_S1_ASIDPNE;
> + if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> + reg |= SCTLR_E;
> + }
> writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
> }
>
> @@ -1035,6 +1066,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>
> /* Initialise the context bank with our page table cfg */
> arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
>
> /*
> * Request context fault interrupt. Do this last to avoid the
> @@ -1067,7 +1099,6 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> struct arm_smmu_device *smmu = smmu_domain->smmu;
> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> - void __iomem *cb_base;
> int irq;
>
> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
> @@ -1077,8 +1108,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
> * Disable the context bank and free the page tables before freeing
> * it.
> */
> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
> - writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
> + smmu->cbs[cfg->cbndx] = (struct arm_smmu_cb){0};
> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
So here, we end up passing a zeroed structure to
arm_smmu_write_context_bank, but afaict that will write to the TTBR and TCR
before SCTLR, which worries me. Couldn't we get whacky speculative table
walks through physical address 0 with ASID 0 before we kill the SCTLR?
If the only time we pass something with a NULL cfg is on the destroy path,
perhaps just do:
if (!cfg) {
writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
return;
}
instead?
Will
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] iommu/arm-smmu: Add system PM support
2017-07-18 12:44 ` Robin Murphy
@ 2017-08-08 11:18 ` Will Deacon
-1 siblings, 0 replies; 18+ messages in thread
From: Will Deacon @ 2017-08-08 11:18 UTC (permalink / raw)
To: Robin Murphy
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Tue, Jul 18, 2017 at 01:44:42PM +0100, Robin Murphy wrote:
> With all our hardware state tracked in such a way that we can naturally
> restore it as part of the necessary reset, resuming is trivial, and
> there's nothing to do on suspend at all.
>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> drivers/iommu/arm-smmu.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 86897b7b81d8..0f5f06e9abfa 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -2356,10 +2356,22 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> +{
Did you actually get a warning here without the __maybe_unused annotation?
It looks like some other drivers just guard the thing with CONFIG_PM_SLEEP.
> + struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +
> + arm_smmu_device_reset(smmu);
> + return 0;
> +}
> +
> +
> +static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
> +
> static struct platform_driver arm_smmu_driver = {
> .driver = {
> .name = "arm-smmu",
> .of_match_table = of_match_ptr(arm_smmu_of_match),
> + .pm = &arm_smmu_pm_ops,
Cosmetic: can you tab-align this assignment please?
Will
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] iommu/arm-smmu: Add system PM support
@ 2017-08-08 11:18 ` Will Deacon
0 siblings, 0 replies; 18+ messages in thread
From: Will Deacon @ 2017-08-08 11:18 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jul 18, 2017 at 01:44:42PM +0100, Robin Murphy wrote:
> With all our hardware state tracked in such a way that we can naturally
> restore it as part of the necessary reset, resuming is trivial, and
> there's nothing to do on suspend at all.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> drivers/iommu/arm-smmu.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 86897b7b81d8..0f5f06e9abfa 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -2356,10 +2356,22 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> +{
Did you actually get a warning here without the __maybe_unused annotation?
It looks like some other drivers just guard the thing with CONFIG_PM_SLEEP.
> + struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +
> + arm_smmu_device_reset(smmu);
> + return 0;
> +}
> +
> +
> +static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
> +
> static struct platform_driver arm_smmu_driver = {
> .driver = {
> .name = "arm-smmu",
> .of_match_table = of_match_ptr(arm_smmu_of_match),
> + .pm = &arm_smmu_pm_ops,
Cosmetic: can you tab-align this assignment please?
Will
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] iommu/arm-smmu: Track context bank state
2017-08-08 11:11 ` Will Deacon
@ 2017-08-08 12:06 ` Robin Murphy
-1 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2017-08-08 12:06 UTC (permalink / raw)
To: Will Deacon
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 08/08/17 12:11, Will Deacon wrote:
> Hi Robin,
>
> I like the idea, but I think there are a few minor issues with the patch.
> Comments below.
>
> On Tue, Jul 18, 2017 at 01:44:41PM +0100, Robin Murphy wrote:
>> Echoing what we do for Stream Map Entries, maintain a software shadow
>> state for context bank configuration. With this in place, we are mere
>> moments away from blissfully easy suspend/resume support.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>
>> Since the runtime PM discussion has come back again, I figured it was
>> probably about time to finish off my plan for system PM. Lightly tested
>> on Juno (MMU-401) with hibernation.
>>
>> Robin.
>>
>> drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++++------------------
>> 1 file changed, 97 insertions(+), 62 deletions(-)
>
> [...]
>
>> +static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
>> +{
>> + u32 reg;
>> + bool stage1;
>> + struct arm_smmu_cb *cb = &smmu->cbs[idx];
>> + struct arm_smmu_cfg *cfg = cb->cfg;
>> + struct arm_smmu_cfg default_cfg = {0};
>> void __iomem *cb_base, *gr1_base;
>>
>> + if (!cfg)
>> + cfg = &default_cfg;
>> +
>> gr1_base = ARM_SMMU_GR1(smmu);
>> stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
>> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
>> + cb_base = ARM_SMMU_CB(smmu, idx);
>>
>> + /* CBA2R */
>> if (smmu->version > ARM_SMMU_V1) {
>> if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>> reg = CBA2R_RW64_64BIT;
>> @@ -788,7 +848,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>> if (smmu->features & ARM_SMMU_FEAT_VMID16)
>> reg |= cfg->vmid << CBA2R_VMID_SHIFT;
>>
>> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
>> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(idx));
>> }
>>
>> /* CBAR */
>> @@ -807,72 +867,43 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>> /* 8-bit VMIDs live in CBAR */
>> reg |= cfg->vmid << CBAR_VMID_SHIFT;
>> }
>> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
>> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(idx));
>>
>> /*
>> * TTBCR
>> * We must write this before the TTBRs, since it determines the
>> * access behaviour of some fields (in particular, ASID[15:8]).
>> */
>> - if (stage1) {
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> - reg = pgtbl_cfg->arm_v7s_cfg.tcr;
>> - reg2 = 0;
>> - } else {
>> - reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
>> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
>> - reg2 |= TTBCR2_SEP_UPSTREAM;
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>> - reg2 |= TTBCR2_AS;
>> - }
>> - if (smmu->version > ARM_SMMU_V1)
>> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
>> - } else {
>> - reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
>> - }
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
>> + if (stage1 && smmu->version > ARM_SMMU_V1)
>> + writel_relaxed(cb->tcr[1], cb_base + ARM_SMMU_CB_TTBCR2);
>> + writel_relaxed(cb->tcr[0], cb_base + ARM_SMMU_CB_TTBCR);
>>
>> /* TTBRs */
>> - if (stage1) {
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0);
>> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1);
>> - writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
>> - } else {
>> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
>> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
>> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
>> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
>> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
>> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR1);
>> - }
>> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> + writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
>> + writel_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
>> + writel_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
>> } else {
>> - reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
>> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
>> + writeq_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
>> + writeq_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
>
> This doesn't look right to me. For the LPAE S1 case, we don't set the ASID
> afaict, and for the S2 case we write to a RESERVED register (since we only
> have one TTBR).
Oops, yes, arm_smmu_init_context_bank() has indeed forgotten to munge
the ASID into cb->ttbr[0] for that case.
TTBR1, though, is not an oversight - architecturally it is UNK/SBZP in
stage 2 contexts, so since we never read it and cb->ttbr[1] will always
be zero for stage 2, this is a valid thing to do and helps keep the code
simple.
>> }
>>
>> /* MAIRs (stage-1 only) */
>> if (stage1) {
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> - reg = pgtbl_cfg->arm_v7s_cfg.prrr;
>> - reg2 = pgtbl_cfg->arm_v7s_cfg.nmrr;
>> - } else {
>> - reg = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
>> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
>> - }
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_S1_MAIR0);
>> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_S1_MAIR1);
>> + writel_relaxed(cb->mair[0], cb_base + ARM_SMMU_CB_S1_MAIR0);
>> + writel_relaxed(cb->mair[1], cb_base + ARM_SMMU_CB_S1_MAIR1);
>> }
>>
>> /* SCTLR */
>> - reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
>> - if (stage1)
>> - reg |= SCTLR_S1_ASIDPNE;
>> -#ifdef __BIG_ENDIAN
>> - reg |= SCTLR_E;
>> -#endif
>> + if (!cb->cfg) {
>> + reg = 0;
>
> I think this might be too late. See below.
Ha, this bit did feel slightly awkward, and you are indeed quite right...
>> + } else {
>> + reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
>> + if (stage1)
>> + reg |= SCTLR_S1_ASIDPNE;
>> + if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>> + reg |= SCTLR_E;
>> + }
>> writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
>> }
>>
>> @@ -1035,6 +1066,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>>
>> /* Initialise the context bank with our page table cfg */
>> arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
>> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
>>
>> /*
>> * Request context fault interrupt. Do this last to avoid the
>> @@ -1067,7 +1099,6 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> struct arm_smmu_device *smmu = smmu_domain->smmu;
>> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>> - void __iomem *cb_base;
>> int irq;
>>
>> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
>> @@ -1077,8 +1108,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>> * Disable the context bank and free the page tables before freeing
>> * it.
>> */
>> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
>> - writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
>> + smmu->cbs[cfg->cbndx] = (struct arm_smmu_cb){0};
>> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
>
> So here, we end up passing a zeroed structure to
> arm_smmu_write_context_bank, but afaict that will write to the TTBR and TCR
> before SCTLR, which worries me. Couldn't we get whacky speculative table
> walks through physical address 0 with ASID 0 before we kill the SCTLR?
>
> If the only time we pass something with a NULL cfg is on the destroy path,
> perhaps just do:
>
> if (!cfg) {
> writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
> return;
> }
>
> instead?
We also hit all banks with a NULL cfg in the initial reset from
arm_smmu_device_probe(), but that still has the same semantics as
teardown - if the context isn't assigned to a domain, there's no point
even touching any hardware registers other than clearing SCTLR, so
there's no need for the whole default_cfg silliness at all. Thanks for
helping me see the obvious!
Robin.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] iommu/arm-smmu: Track context bank state
@ 2017-08-08 12:06 ` Robin Murphy
0 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2017-08-08 12:06 UTC (permalink / raw)
To: linux-arm-kernel
On 08/08/17 12:11, Will Deacon wrote:
> Hi Robin,
>
> I like the idea, but I think there are a few minor issues with the patch.
> Comments below.
>
> On Tue, Jul 18, 2017 at 01:44:41PM +0100, Robin Murphy wrote:
>> Echoing what we do for Stream Map Entries, maintain a software shadow
>> state for context bank configuration. With this in place, we are mere
>> moments away from blissfully easy suspend/resume support.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> Since the runtime PM discussion has come back again, I figured it was
>> probably about time to finish off my plan for system PM. Lightly tested
>> on Juno (MMU-401) with hibernation.
>>
>> Robin.
>>
>> drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++++------------------
>> 1 file changed, 97 insertions(+), 62 deletions(-)
>
> [...]
>
>> +static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
>> +{
>> + u32 reg;
>> + bool stage1;
>> + struct arm_smmu_cb *cb = &smmu->cbs[idx];
>> + struct arm_smmu_cfg *cfg = cb->cfg;
>> + struct arm_smmu_cfg default_cfg = {0};
>> void __iomem *cb_base, *gr1_base;
>>
>> + if (!cfg)
>> + cfg = &default_cfg;
>> +
>> gr1_base = ARM_SMMU_GR1(smmu);
>> stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
>> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
>> + cb_base = ARM_SMMU_CB(smmu, idx);
>>
>> + /* CBA2R */
>> if (smmu->version > ARM_SMMU_V1) {
>> if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>> reg = CBA2R_RW64_64BIT;
>> @@ -788,7 +848,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>> if (smmu->features & ARM_SMMU_FEAT_VMID16)
>> reg |= cfg->vmid << CBA2R_VMID_SHIFT;
>>
>> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(cfg->cbndx));
>> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBA2R(idx));
>> }
>>
>> /* CBAR */
>> @@ -807,72 +867,43 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>> /* 8-bit VMIDs live in CBAR */
>> reg |= cfg->vmid << CBAR_VMID_SHIFT;
>> }
>> - writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
>> + writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(idx));
>>
>> /*
>> * TTBCR
>> * We must write this before the TTBRs, since it determines the
>> * access behaviour of some fields (in particular, ASID[15:8]).
>> */
>> - if (stage1) {
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> - reg = pgtbl_cfg->arm_v7s_cfg.tcr;
>> - reg2 = 0;
>> - } else {
>> - reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
>> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
>> - reg2 |= TTBCR2_SEP_UPSTREAM;
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
>> - reg2 |= TTBCR2_AS;
>> - }
>> - if (smmu->version > ARM_SMMU_V1)
>> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
>> - } else {
>> - reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
>> - }
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
>> + if (stage1 && smmu->version > ARM_SMMU_V1)
>> + writel_relaxed(cb->tcr[1], cb_base + ARM_SMMU_CB_TTBCR2);
>> + writel_relaxed(cb->tcr[0], cb_base + ARM_SMMU_CB_TTBCR);
>>
>> /* TTBRs */
>> - if (stage1) {
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0);
>> - reg = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1);
>> - writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
>> - } else {
>> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
>> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
>> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
>> - reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
>> - reg64 |= (u64)cfg->asid << TTBRn_ASID_SHIFT;
>> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR1);
>> - }
>> + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> + writel_relaxed(cfg->asid, cb_base + ARM_SMMU_CB_CONTEXTIDR);
>> + writel_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
>> + writel_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
>> } else {
>> - reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
>> - writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
>> + writeq_relaxed(cb->ttbr[0], cb_base + ARM_SMMU_CB_TTBR0);
>> + writeq_relaxed(cb->ttbr[1], cb_base + ARM_SMMU_CB_TTBR1);
>
> This doesn't look right to me. For the LPAE S1 case, we don't set the ASID
> afaict, and for the S2 case we write to a RESERVED register (since we only
> have one TTBR).
Oops, yes, arm_smmu_init_context_bank() has indeed forgotten to munge
the ASID into cb->ttbr[0] for that case.
TTBR1, though, is not an oversight - architecturally it is UNK/SBZP in
stage 2 contexts, so since we never read it and cb->ttbr[1] will always
be zero for stage 2, this is a valid thing to do and helps keep the code
simple.
>> }
>>
>> /* MAIRs (stage-1 only) */
>> if (stage1) {
>> - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
>> - reg = pgtbl_cfg->arm_v7s_cfg.prrr;
>> - reg2 = pgtbl_cfg->arm_v7s_cfg.nmrr;
>> - } else {
>> - reg = pgtbl_cfg->arm_lpae_s1_cfg.mair[0];
>> - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.mair[1];
>> - }
>> - writel_relaxed(reg, cb_base + ARM_SMMU_CB_S1_MAIR0);
>> - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_S1_MAIR1);
>> + writel_relaxed(cb->mair[0], cb_base + ARM_SMMU_CB_S1_MAIR0);
>> + writel_relaxed(cb->mair[1], cb_base + ARM_SMMU_CB_S1_MAIR1);
>> }
>>
>> /* SCTLR */
>> - reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
>> - if (stage1)
>> - reg |= SCTLR_S1_ASIDPNE;
>> -#ifdef __BIG_ENDIAN
>> - reg |= SCTLR_E;
>> -#endif
>> + if (!cb->cfg) {
>> + reg = 0;
>
> I think this might be too late. See below.
Ha, this bit did feel slightly awkward, and you are indeed quite right...
>> + } else {
>> + reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
>> + if (stage1)
>> + reg |= SCTLR_S1_ASIDPNE;
>> + if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>> + reg |= SCTLR_E;
>> + }
>> writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
>> }
>>
>> @@ -1035,6 +1066,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>>
>> /* Initialise the context bank with our page table cfg */
>> arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
>> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
>>
>> /*
>> * Request context fault interrupt. Do this last to avoid the
>> @@ -1067,7 +1099,6 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> struct arm_smmu_device *smmu = smmu_domain->smmu;
>> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>> - void __iomem *cb_base;
>> int irq;
>>
>> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
>> @@ -1077,8 +1108,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>> * Disable the context bank and free the page tables before freeing
>> * it.
>> */
>> - cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
>> - writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
>> + smmu->cbs[cfg->cbndx] = (struct arm_smmu_cb){0};
>> + arm_smmu_write_context_bank(smmu, cfg->cbndx);
>
> So here, we end up passing a zeroed structure to
> arm_smmu_write_context_bank, but afaict that will write to the TTBR and TCR
> before SCTLR, which worries me. Couldn't we get whacky speculative table
> walks through physical address 0 with ASID 0 before we kill the SCTLR?
>
> If the only time we pass something with a NULL cfg is on the destroy path,
> perhaps just do:
>
> if (!cfg) {
> writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
> return;
> }
>
> instead?
We also hit all banks with a NULL cfg in the initial reset from
arm_smmu_device_probe(), but that still has the same semantics as
teardown - if the context isn't assigned to a domain, there's no point
even touching any hardware registers other than clearing SCTLR, so
there's no need for the whole default_cfg silliness at all. Thanks for
helping me see the obvious!
Robin.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] iommu/arm-smmu: Add system PM support
2017-08-08 11:18 ` Will Deacon
@ 2017-08-08 12:14 ` Robin Murphy
-1 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2017-08-08 12:14 UTC (permalink / raw)
To: Will Deacon; +Cc: vivek.gautam, joro, sricharan, iommu, linux-arm-kernel
On 08/08/17 12:18, Will Deacon wrote:
> On Tue, Jul 18, 2017 at 01:44:42PM +0100, Robin Murphy wrote:
>> With all our hardware state tracked in such a way that we can naturally
>> restore it as part of the necessary reset, resuming is trivial, and
>> there's nothing to do on suspend at all.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>> drivers/iommu/arm-smmu.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 86897b7b81d8..0f5f06e9abfa 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -2356,10 +2356,22 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
>> +{
>
> Did you actually get a warning here without the __maybe_unused annotation?
> It looks like some other drivers just guard the thing with CONFIG_PM_SLEEP.
I'm under the impression that the annotation is preferred over #ifdefs
for new code (for the sake of coverage, I guess).
>> + struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>> +
>> + arm_smmu_device_reset(smmu);
>> + return 0;
>> +}
>> +
>> +
>> +static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
>> +
>> static struct platform_driver arm_smmu_driver = {
>> .driver = {
>> .name = "arm-smmu",
>> .of_match_table = of_match_ptr(arm_smmu_of_match),
>> + .pm = &arm_smmu_pm_ops,
>
> Cosmetic: can you tab-align this assignment please?
Oops, I missed that - will do.
Robin.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] iommu/arm-smmu: Add system PM support
@ 2017-08-08 12:14 ` Robin Murphy
0 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2017-08-08 12:14 UTC (permalink / raw)
To: linux-arm-kernel
On 08/08/17 12:18, Will Deacon wrote:
> On Tue, Jul 18, 2017 at 01:44:42PM +0100, Robin Murphy wrote:
>> With all our hardware state tracked in such a way that we can naturally
>> restore it as part of the necessary reset, resuming is trivial, and
>> there's nothing to do on suspend at all.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>> drivers/iommu/arm-smmu.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 86897b7b81d8..0f5f06e9abfa 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -2356,10 +2356,22 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
>> +{
>
> Did you actually get a warning here without the __maybe_unused annotation?
> It looks like some other drivers just guard the thing with CONFIG_PM_SLEEP.
I'm under the impression that the annotation is preferred over #ifdefs
for new code (for the sake of coverage, I guess).
>> + struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>> +
>> + arm_smmu_device_reset(smmu);
>> + return 0;
>> +}
>> +
>> +
>> +static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
>> +
>> static struct platform_driver arm_smmu_driver = {
>> .driver = {
>> .name = "arm-smmu",
>> .of_match_table = of_match_ptr(arm_smmu_of_match),
>> + .pm = &arm_smmu_pm_ops,
>
> Cosmetic: can you tab-align this assignment please?
Oops, I missed that - will do.
Robin.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] iommu/arm-smmu: Add system PM support
2017-08-08 12:14 ` Robin Murphy
@ 2017-08-11 10:04 ` Jonathan Cameron
-1 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2017-08-11 10:04 UTC (permalink / raw)
To: Robin Murphy
Cc: Will Deacon, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Tue, 8 Aug 2017 13:14:18 +0100
Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
> On 08/08/17 12:18, Will Deacon wrote:
> > On Tue, Jul 18, 2017 at 01:44:42PM +0100, Robin Murphy wrote:
> >> With all our hardware state tracked in such a way that we can naturally
> >> restore it as part of the necessary reset, resuming is trivial, and
> >> there's nothing to do on suspend at all.
> >>
> >> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> >> ---
> >> drivers/iommu/arm-smmu.c | 12 ++++++++++++
> >> 1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >> index 86897b7b81d8..0f5f06e9abfa 100644
> >> --- a/drivers/iommu/arm-smmu.c
> >> +++ b/drivers/iommu/arm-smmu.c
> >> @@ -2356,10 +2356,22 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
> >> return 0;
> >> }
> >>
> >> +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> >> +{
> >
> > Did you actually get a warning here without the __maybe_unused annotation?
> > It looks like some other drivers just guard the thing with CONFIG_PM_SLEEP.
>
> I'm under the impression that the annotation is preferred over #ifdefs
> for new code (for the sake of coverage, I guess).
https://patchwork.kernel.org/patch/9734367/
Is a good thread discussing this. Both coverage and to avoid common
pitfalls of the ifdef fun.
Jonathan
>
> >> + struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> >> +
> >> + arm_smmu_device_reset(smmu);
> >> + return 0;
> >> +}
> >> +
> >> +
> >> +static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
> >> +
> >> static struct platform_driver arm_smmu_driver = {
> >> .driver = {
> >> .name = "arm-smmu",
> >> .of_match_table = of_match_ptr(arm_smmu_of_match),
> >> + .pm = &arm_smmu_pm_ops,
> >
> > Cosmetic: can you tab-align this assignment please?
>
> Oops, I missed that - will do.
>
> Robin.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] iommu/arm-smmu: Add system PM support
@ 2017-08-11 10:04 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2017-08-11 10:04 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 8 Aug 2017 13:14:18 +0100
Robin Murphy <robin.murphy@arm.com> wrote:
> On 08/08/17 12:18, Will Deacon wrote:
> > On Tue, Jul 18, 2017 at 01:44:42PM +0100, Robin Murphy wrote:
> >> With all our hardware state tracked in such a way that we can naturally
> >> restore it as part of the necessary reset, resuming is trivial, and
> >> there's nothing to do on suspend at all.
> >>
> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> >> ---
> >> drivers/iommu/arm-smmu.c | 12 ++++++++++++
> >> 1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >> index 86897b7b81d8..0f5f06e9abfa 100644
> >> --- a/drivers/iommu/arm-smmu.c
> >> +++ b/drivers/iommu/arm-smmu.c
> >> @@ -2356,10 +2356,22 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
> >> return 0;
> >> }
> >>
> >> +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> >> +{
> >
> > Did you actually get a warning here without the __maybe_unused annotation?
> > It looks like some other drivers just guard the thing with CONFIG_PM_SLEEP.
>
> I'm under the impression that the annotation is preferred over #ifdefs
> for new code (for the sake of coverage, I guess).
https://patchwork.kernel.org/patch/9734367/
Is a good thread discussing this. Both coverage and to avoid common
pitfalls of the ifdef fun.
Jonathan
>
> >> + struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> >> +
> >> + arm_smmu_device_reset(smmu);
> >> + return 0;
> >> +}
> >> +
> >> +
> >> +static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
> >> +
> >> static struct platform_driver arm_smmu_driver = {
> >> .driver = {
> >> .name = "arm-smmu",
> >> .of_match_table = of_match_ptr(arm_smmu_of_match),
> >> + .pm = &arm_smmu_pm_ops,
> >
> > Cosmetic: can you tab-align this assignment please?
>
> Oops, I missed that - will do.
>
> Robin.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-08-11 10:04 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-18 12:44 [PATCH 1/2] iommu/arm-smmu: Track context bank state Robin Murphy
2017-07-18 12:44 ` Robin Murphy
[not found] ` <71247263f4d88e7776f483fb1cc1139b516c0835.1500381551.git.robin.murphy-5wv7dgnIgG8@public.gmane.org>
2017-07-18 12:44 ` [PATCH 2/2] iommu/arm-smmu: Add system PM support Robin Murphy
2017-07-18 12:44 ` Robin Murphy
[not found] ` <d8d6f3b606ed05d53b47d36e1c6a7f4b74eb3afc.1500381551.git.robin.murphy-5wv7dgnIgG8@public.gmane.org>
2017-08-08 11:18 ` Will Deacon
2017-08-08 11:18 ` Will Deacon
2017-08-08 12:14 ` Robin Murphy
2017-08-08 12:14 ` Robin Murphy
[not found] ` <7d0d846b-8ada-6514-ec27-22f4d8487014-5wv7dgnIgG8@public.gmane.org>
2017-08-11 10:04 ` Jonathan Cameron
2017-08-11 10:04 ` Jonathan Cameron
2017-07-19 2:40 ` [PATCH 1/2] iommu/arm-smmu: Track context bank state Sricharan R
2017-07-19 2:40 ` Sricharan R
[not found] ` <22b7eea9-d194-d768-a10f-fc3c6fec9e4c-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2017-07-19 10:17 ` Robin Murphy
2017-07-19 10:17 ` Robin Murphy
2017-08-08 11:11 ` Will Deacon
2017-08-08 11:11 ` Will Deacon
[not found] ` <20170808111151.GC13355-5wv7dgnIgG8@public.gmane.org>
2017-08-08 12:06 ` Robin Murphy
2017-08-08 12:06 ` Robin Murphy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.