* [PATCH 1/7] iommu/ipmmu-vmsa: Link IOMMUs and devices in sysfs
2019-02-20 15:05 [PATCH 0/7] iommu/ipmmu-vmsa: Suspend/resume support and assorted cleanups Geert Uytterhoeven
@ 2019-02-20 15:05 ` Geert Uytterhoeven
2019-02-20 15:25 ` Laurent Pinchart
2019-02-20 15:05 ` [PATCH 2/7] iommu/ipmmu-vmsa: Call ipmmu_ctx_write_root() instead of open coding Geert Uytterhoeven
` (5 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2019-02-20 15:05 UTC (permalink / raw)
To: Joerg Roedel, Magnus Damm
Cc: Laurent Pinchart, iommu, linux-renesas-soc, linux-kernel,
Geert Uytterhoeven
As of commit 7af9a5fdb9e0ca33 ("iommu/ipmmu-vmsa: Use
iommu_device_sysfs_add()/remove()"), IOMMU devices show up under
/sys/class/iommus/, but their "devices" subdirectories are empty.
Likewise, devices tied to an IOMMU do not have an "iommu" backlink.
Make sure all links are created, on both arm32 and arm64.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
drivers/iommu/ipmmu-vmsa.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 9a380c10655e182d..9f2b781e20a0eba6 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -885,27 +885,37 @@ static int ipmmu_init_arm_mapping(struct device *dev)
static int ipmmu_add_device(struct device *dev)
{
+ struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
struct iommu_group *group;
+ int ret;
/*
* Only let through devices that have been verified in xlate()
*/
- if (!to_ipmmu(dev))
+ if (!mmu)
return -ENODEV;
- if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_IOMMU_DMA))
- return ipmmu_init_arm_mapping(dev);
+ if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_IOMMU_DMA)) {
+ ret = ipmmu_init_arm_mapping(dev);
+ if (ret)
+ return ret;
+ } else {
+ group = iommu_group_get_for_dev(dev);
+ if (IS_ERR(group))
+ return PTR_ERR(group);
- group = iommu_group_get_for_dev(dev);
- if (IS_ERR(group))
- return PTR_ERR(group);
+ iommu_group_put(group);
+ }
- iommu_group_put(group);
+ iommu_device_link(&mmu->iommu, dev);
return 0;
}
static void ipmmu_remove_device(struct device *dev)
{
+ struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
+
+ iommu_device_unlink(&mmu->iommu, dev);
arm_iommu_detach_device(dev);
iommu_group_remove_device(dev);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 1/7] iommu/ipmmu-vmsa: Link IOMMUs and devices in sysfs
2019-02-20 15:05 ` [PATCH 1/7] iommu/ipmmu-vmsa: Link IOMMUs and devices in sysfs Geert Uytterhoeven
@ 2019-02-20 15:25 ` Laurent Pinchart
0 siblings, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2019-02-20 15:25 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Joerg Roedel, Magnus Damm, Laurent Pinchart, iommu,
linux-renesas-soc, linux-kernel
Hi Geert,
Thank you for the patch.
On Wed, Feb 20, 2019 at 04:05:25PM +0100, Geert Uytterhoeven wrote:
> As of commit 7af9a5fdb9e0ca33 ("iommu/ipmmu-vmsa: Use
> iommu_device_sysfs_add()/remove()"), IOMMU devices show up under
> /sys/class/iommus/, but their "devices" subdirectories are empty.
> Likewise, devices tied to an IOMMU do not have an "iommu" backlink.
>
> Make sure all links are created, on both arm32 and arm64.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/iommu/ipmmu-vmsa.c | 24 +++++++++++++++++-------
> 1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 9a380c10655e182d..9f2b781e20a0eba6 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -885,27 +885,37 @@ static int ipmmu_init_arm_mapping(struct device *dev)
>
> static int ipmmu_add_device(struct device *dev)
> {
> + struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
> struct iommu_group *group;
> + int ret;
>
> /*
> * Only let through devices that have been verified in xlate()
> */
> - if (!to_ipmmu(dev))
> + if (!mmu)
> return -ENODEV;
>
> - if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_IOMMU_DMA))
> - return ipmmu_init_arm_mapping(dev);
> + if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_IOMMU_DMA)) {
> + ret = ipmmu_init_arm_mapping(dev);
> + if (ret)
> + return ret;
> + } else {
> + group = iommu_group_get_for_dev(dev);
> + if (IS_ERR(group))
> + return PTR_ERR(group);
>
> - group = iommu_group_get_for_dev(dev);
> - if (IS_ERR(group))
> - return PTR_ERR(group);
> + iommu_group_put(group);
> + }
>
> - iommu_group_put(group);
> + iommu_device_link(&mmu->iommu, dev);
> return 0;
> }
>
> static void ipmmu_remove_device(struct device *dev)
> {
> + struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
> +
> + iommu_device_unlink(&mmu->iommu, dev);
> arm_iommu_detach_device(dev);
> iommu_group_remove_device(dev);
> }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/7] iommu/ipmmu-vmsa: Call ipmmu_ctx_write_root() instead of open coding
2019-02-20 15:05 [PATCH 0/7] iommu/ipmmu-vmsa: Suspend/resume support and assorted cleanups Geert Uytterhoeven
2019-02-20 15:05 ` [PATCH 1/7] iommu/ipmmu-vmsa: Link IOMMUs and devices in sysfs Geert Uytterhoeven
@ 2019-02-20 15:05 ` Geert Uytterhoeven
2019-02-20 15:27 ` Laurent Pinchart
2019-02-20 15:05 ` [PATCH 3/7] iommu/ipmmu-vmsa: Prepare to handle 40-bit error addresses Geert Uytterhoeven
` (4 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2019-02-20 15:05 UTC (permalink / raw)
To: Joerg Roedel, Magnus Damm
Cc: Laurent Pinchart, iommu, linux-renesas-soc, linux-kernel,
Geert Uytterhoeven
There is a helper to write to the root IPMMU instance's registers, so
let's use it.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
drivers/iommu/ipmmu-vmsa.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 9f2b781e20a0eba6..ac70cb967ff821c6 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -280,8 +280,7 @@ static void ipmmu_ctx_write_all(struct ipmmu_vmsa_domain *domain,
ipmmu_write(domain->mmu,
domain->context_id * IM_CTX_SIZE + reg, data);
- ipmmu_write(domain->mmu->root,
- domain->context_id * IM_CTX_SIZE + reg, data);
+ ipmmu_ctx_write_root(domain, reg, data);
}
/* -----------------------------------------------------------------------------
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 2/7] iommu/ipmmu-vmsa: Call ipmmu_ctx_write_root() instead of open coding
2019-02-20 15:05 ` [PATCH 2/7] iommu/ipmmu-vmsa: Call ipmmu_ctx_write_root() instead of open coding Geert Uytterhoeven
@ 2019-02-20 15:27 ` Laurent Pinchart
0 siblings, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2019-02-20 15:27 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Joerg Roedel, Magnus Damm, Laurent Pinchart, iommu,
linux-renesas-soc, linux-kernel
Hi Geert,
Thank you for the patch.
On Wed, Feb 20, 2019 at 04:05:26PM +0100, Geert Uytterhoeven wrote:
> There is a helper to write to the root IPMMU instance's registers, so
> let's use it.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> drivers/iommu/ipmmu-vmsa.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 9f2b781e20a0eba6..ac70cb967ff821c6 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -280,8 +280,7 @@ static void ipmmu_ctx_write_all(struct ipmmu_vmsa_domain *domain,
> ipmmu_write(domain->mmu,
> domain->context_id * IM_CTX_SIZE + reg, data);
>
> - ipmmu_write(domain->mmu->root,
> - domain->context_id * IM_CTX_SIZE + reg, data);
> + ipmmu_ctx_write_root(domain, reg, data);
This makes the code harder to read in my opinion, and I don't think it
bring much optimization.
> }
>
> /* -----------------------------------------------------------------------------
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/7] iommu/ipmmu-vmsa: Prepare to handle 40-bit error addresses
2019-02-20 15:05 [PATCH 0/7] iommu/ipmmu-vmsa: Suspend/resume support and assorted cleanups Geert Uytterhoeven
2019-02-20 15:05 ` [PATCH 1/7] iommu/ipmmu-vmsa: Link IOMMUs and devices in sysfs Geert Uytterhoeven
2019-02-20 15:05 ` [PATCH 2/7] iommu/ipmmu-vmsa: Call ipmmu_ctx_write_root() instead of open coding Geert Uytterhoeven
@ 2019-02-20 15:05 ` Geert Uytterhoeven
2019-02-20 15:30 ` Laurent Pinchart
2019-02-20 15:05 ` [PATCH 4/7] iommu/ipmmu-vmsa: Make IPMMU_CTX_MAX unsigned Geert Uytterhoeven
` (3 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2019-02-20 15:05 UTC (permalink / raw)
To: Joerg Roedel, Magnus Damm
Cc: Laurent Pinchart, iommu, linux-renesas-soc, linux-kernel,
Geert Uytterhoeven
On R-Car Gen3, the faulting virtual address is a 40-bit address, and
comprised of two registers. Read the upper address part, and combine
both parts, when running on a 64-bit system.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Apart from this, the driver doesn't support 40-bit IOVA addresses yet.
---
drivers/iommu/ipmmu-vmsa.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index ac70cb967ff821c6..4d07c26c97848b65 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -186,7 +186,9 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device *dev)
#define IMMAIR_ATTR_IDX_WBRWA 1
#define IMMAIR_ATTR_IDX_DEV 2
-#define IMEAR 0x0030
+#define IMEAR 0x0030 /* R-Car Gen2 */
+#define IMELAR IMEAR /* R-Car Gen3 */
+#define IMEUAR 0x0034 /* R-Car Gen3 */
#define IMPCTR 0x0200
#define IMPSTR 0x0208
@@ -521,14 +523,16 @@ static irqreturn_t ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
{
const u32 err_mask = IMSTR_MHIT | IMSTR_ABORT | IMSTR_PF | IMSTR_TF;
struct ipmmu_vmsa_device *mmu = domain->mmu;
+ unsigned long iova;
u32 status;
- u32 iova;
status = ipmmu_ctx_read_root(domain, IMSTR);
if (!(status & err_mask))
return IRQ_NONE;
- iova = ipmmu_ctx_read_root(domain, IMEAR);
+ iova = ipmmu_ctx_read_root(domain, IMELAR);
+ if (IS_ENABLED(CONFIG_64BIT))
+ iova |= (u64)ipmmu_ctx_read_root(domain, IMEUAR) << 32;
/*
* Clear the error status flags. Unlike traditional interrupt flag
@@ -540,10 +544,10 @@ static irqreturn_t ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
/* Log fatal errors. */
if (status & IMSTR_MHIT)
- dev_err_ratelimited(mmu->dev, "Multiple TLB hits @0x%08x\n",
+ dev_err_ratelimited(mmu->dev, "Multiple TLB hits @0x%lx\n",
iova);
if (status & IMSTR_ABORT)
- dev_err_ratelimited(mmu->dev, "Page Table Walk Abort @0x%08x\n",
+ dev_err_ratelimited(mmu->dev, "Page Table Walk Abort @0x%lx\n",
iova);
if (!(status & (IMSTR_PF | IMSTR_TF)))
@@ -559,7 +563,7 @@ static irqreturn_t ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
return IRQ_HANDLED;
dev_err_ratelimited(mmu->dev,
- "Unhandled fault: status 0x%08x iova 0x%08x\n",
+ "Unhandled fault: status 0x%08x iova 0x%lx\n",
status, iova);
return IRQ_HANDLED;
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 3/7] iommu/ipmmu-vmsa: Prepare to handle 40-bit error addresses
2019-02-20 15:05 ` [PATCH 3/7] iommu/ipmmu-vmsa: Prepare to handle 40-bit error addresses Geert Uytterhoeven
@ 2019-02-20 15:30 ` Laurent Pinchart
2019-02-20 15:42 ` Geert Uytterhoeven
0 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2019-02-20 15:30 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Joerg Roedel, Magnus Damm, Laurent Pinchart, iommu,
linux-renesas-soc, linux-kernel
Hi Geert,
Thank you for the patch.
On Wed, Feb 20, 2019 at 04:05:27PM +0100, Geert Uytterhoeven wrote:
> On R-Car Gen3, the faulting virtual address is a 40-bit address, and
> comprised of two registers. Read the upper address part, and combine
> both parts, when running on a 64-bit system.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Apart from this, the driver doesn't support 40-bit IOVA addresses yet.
> ---
> drivers/iommu/ipmmu-vmsa.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index ac70cb967ff821c6..4d07c26c97848b65 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -186,7 +186,9 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device *dev)
> #define IMMAIR_ATTR_IDX_WBRWA 1
> #define IMMAIR_ATTR_IDX_DEV 2
>
> -#define IMEAR 0x0030
> +#define IMEAR 0x0030 /* R-Car Gen2 */
> +#define IMELAR IMEAR /* R-Car Gen3 */
I would have defined that as a single macro.
> +#define IMEUAR 0x0034 /* R-Car Gen3 */
>
> #define IMPCTR 0x0200
> #define IMPSTR 0x0208
> @@ -521,14 +523,16 @@ static irqreturn_t ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
> {
> const u32 err_mask = IMSTR_MHIT | IMSTR_ABORT | IMSTR_PF | IMSTR_TF;
> struct ipmmu_vmsa_device *mmu = domain->mmu;
> + unsigned long iova;
Isn't there a more appropriate type, such as dma_addr_t possibly ?
> u32 status;
> - u32 iova;
>
> status = ipmmu_ctx_read_root(domain, IMSTR);
> if (!(status & err_mask))
> return IRQ_NONE;
>
> - iova = ipmmu_ctx_read_root(domain, IMEAR);
> + iova = ipmmu_ctx_read_root(domain, IMELAR);
> + if (IS_ENABLED(CONFIG_64BIT))
> + iova |= (u64)ipmmu_ctx_read_root(domain, IMEUAR) << 32;
>
> /*
> * Clear the error status flags. Unlike traditional interrupt flag
> @@ -540,10 +544,10 @@ static irqreturn_t ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
>
> /* Log fatal errors. */
> if (status & IMSTR_MHIT)
> - dev_err_ratelimited(mmu->dev, "Multiple TLB hits @0x%08x\n",
> + dev_err_ratelimited(mmu->dev, "Multiple TLB hits @0x%lx\n",
> iova);
> if (status & IMSTR_ABORT)
> - dev_err_ratelimited(mmu->dev, "Page Table Walk Abort @0x%08x\n",
> + dev_err_ratelimited(mmu->dev, "Page Table Walk Abort @0x%lx\n",
> iova);
>
> if (!(status & (IMSTR_PF | IMSTR_TF)))
> @@ -559,7 +563,7 @@ static irqreturn_t ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
> return IRQ_HANDLED;
>
> dev_err_ratelimited(mmu->dev,
> - "Unhandled fault: status 0x%08x iova 0x%08x\n",
> + "Unhandled fault: status 0x%08x iova 0x%lx\n",
> status, iova);
>
> return IRQ_HANDLED;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/7] iommu/ipmmu-vmsa: Prepare to handle 40-bit error addresses
2019-02-20 15:30 ` Laurent Pinchart
@ 2019-02-20 15:42 ` Geert Uytterhoeven
0 siblings, 0 replies; 21+ messages in thread
From: Geert Uytterhoeven @ 2019-02-20 15:42 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Geert Uytterhoeven, Joerg Roedel, Magnus Damm, Laurent Pinchart,
Linux IOMMU, Linux-Renesas, Linux Kernel Mailing List
Hi Laurent,
On Wed, Feb 20, 2019 at 4:31 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Feb 20, 2019 at 04:05:27PM +0100, Geert Uytterhoeven wrote:
> > On R-Car Gen3, the faulting virtual address is a 40-bit address, and
> > comprised of two registers. Read the upper address part, and combine
> > both parts, when running on a 64-bit system.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > Apart from this, the driver doesn't support 40-bit IOVA addresses yet.
> > ---
> > drivers/iommu/ipmmu-vmsa.c | 16 ++++++++++------
> > 1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> > index ac70cb967ff821c6..4d07c26c97848b65 100644
> > --- a/drivers/iommu/ipmmu-vmsa.c
> > +++ b/drivers/iommu/ipmmu-vmsa.c
> > @@ -186,7 +186,9 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device *dev)
> > #define IMMAIR_ATTR_IDX_WBRWA 1
> > #define IMMAIR_ATTR_IDX_DEV 2
> >
> > -#define IMEAR 0x0030
> > +#define IMEAR 0x0030 /* R-Car Gen2 */
> > +#define IMELAR IMEAR /* R-Car Gen3 */
>
> I would have defined that as a single macro.
Fair enough.
> > +#define IMEUAR 0x0034 /* R-Car Gen3 */
> >
> > #define IMPCTR 0x0200
> > #define IMPSTR 0x0208
> > @@ -521,14 +523,16 @@ static irqreturn_t ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
> > {
> > const u32 err_mask = IMSTR_MHIT | IMSTR_ABORT | IMSTR_PF | IMSTR_TF;
> > struct ipmmu_vmsa_device *mmu = domain->mmu;
> > + unsigned long iova;
>
> Isn't there a more appropriate type, such as dma_addr_t possibly ?
Good question! Thanks for opening the can of worms ;-)
I used "unsigned long", as that's what the "iova" parameter of
report_iommu_fault() (called in this function) takes.
However, dma_addr_t would probably be a better choice.
Note that most iommu_ops methods take unsigned long, except for
.iova_to_phys(), which takes dma_addr_t.
Also, all io_pgtable_ops methods take unsigned long, including
.iova_to_phys().
The latter is interesting, as several IOMMU drivers forward the iova
received in their iommu_ops.iova_to_phys() methods to
io_pgtable_ops.iova_to_phys(), thus truncating the value on e.g. arm32
with LPAE.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/7] iommu/ipmmu-vmsa: Make IPMMU_CTX_MAX unsigned
2019-02-20 15:05 [PATCH 0/7] iommu/ipmmu-vmsa: Suspend/resume support and assorted cleanups Geert Uytterhoeven
` (2 preceding siblings ...)
2019-02-20 15:05 ` [PATCH 3/7] iommu/ipmmu-vmsa: Prepare to handle 40-bit error addresses Geert Uytterhoeven
@ 2019-02-20 15:05 ` Geert Uytterhoeven
2019-02-20 15:31 ` Laurent Pinchart
2019-02-20 15:05 ` [PATCH 5/7] iommu/ipmmu-vmsa: Move num_utlbs to SoC-specific features Geert Uytterhoeven
` (2 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2019-02-20 15:05 UTC (permalink / raw)
To: Joerg Roedel, Magnus Damm
Cc: Laurent Pinchart, iommu, linux-renesas-soc, linux-kernel,
Geert Uytterhoeven
Make the IPMMU_CTX_MAX constant unsigned, to match the type of
ipmmu_features.number_of_contexts.
This allows to use plain min() instead of type-casting min_t().
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
drivers/iommu/ipmmu-vmsa.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 4d07c26c97848b65..4e3134a9a52f8d87 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -36,7 +36,7 @@
#define arm_iommu_detach_device(...) do {} while (0)
#endif
-#define IPMMU_CTX_MAX 8
+#define IPMMU_CTX_MAX 8U
struct ipmmu_features {
bool use_ns_alias_offset;
@@ -1060,8 +1060,7 @@ static int ipmmu_probe(struct platform_device *pdev)
if (mmu->features->use_ns_alias_offset)
mmu->base += IM_NS_ALIAS_OFFSET;
- mmu->num_ctx = min_t(unsigned int, IPMMU_CTX_MAX,
- mmu->features->number_of_contexts);
+ mmu->num_ctx = min(IPMMU_CTX_MAX, mmu->features->number_of_contexts);
irq = platform_get_irq(pdev, 0);
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 4/7] iommu/ipmmu-vmsa: Make IPMMU_CTX_MAX unsigned
2019-02-20 15:05 ` [PATCH 4/7] iommu/ipmmu-vmsa: Make IPMMU_CTX_MAX unsigned Geert Uytterhoeven
@ 2019-02-20 15:31 ` Laurent Pinchart
0 siblings, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2019-02-20 15:31 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Joerg Roedel, Magnus Damm, Laurent Pinchart, iommu,
linux-renesas-soc, linux-kernel
Hi Geert,
Thank you for the patch.
On Wed, Feb 20, 2019 at 04:05:28PM +0100, Geert Uytterhoeven wrote:
> Make the IPMMU_CTX_MAX constant unsigned, to match the type of
> ipmmu_features.number_of_contexts.
>
> This allows to use plain min() instead of type-casting min_t().
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/iommu/ipmmu-vmsa.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 4d07c26c97848b65..4e3134a9a52f8d87 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -36,7 +36,7 @@
> #define arm_iommu_detach_device(...) do {} while (0)
> #endif
>
> -#define IPMMU_CTX_MAX 8
> +#define IPMMU_CTX_MAX 8U
>
> struct ipmmu_features {
> bool use_ns_alias_offset;
> @@ -1060,8 +1060,7 @@ static int ipmmu_probe(struct platform_device *pdev)
> if (mmu->features->use_ns_alias_offset)
> mmu->base += IM_NS_ALIAS_OFFSET;
>
> - mmu->num_ctx = min_t(unsigned int, IPMMU_CTX_MAX,
> - mmu->features->number_of_contexts);
> + mmu->num_ctx = min(IPMMU_CTX_MAX, mmu->features->number_of_contexts);
>
> irq = platform_get_irq(pdev, 0);
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 5/7] iommu/ipmmu-vmsa: Move num_utlbs to SoC-specific features
2019-02-20 15:05 [PATCH 0/7] iommu/ipmmu-vmsa: Suspend/resume support and assorted cleanups Geert Uytterhoeven
` (3 preceding siblings ...)
2019-02-20 15:05 ` [PATCH 4/7] iommu/ipmmu-vmsa: Make IPMMU_CTX_MAX unsigned Geert Uytterhoeven
@ 2019-02-20 15:05 ` Geert Uytterhoeven
2019-02-20 15:42 ` Laurent Pinchart
2019-02-20 15:05 ` [PATCH 6/7] iommu/ipmmu-vmsa: Extract hardware context initialization Geert Uytterhoeven
2019-02-20 15:05 ` [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support Geert Uytterhoeven
6 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2019-02-20 15:05 UTC (permalink / raw)
To: Joerg Roedel, Magnus Damm
Cc: Laurent Pinchart, iommu, linux-renesas-soc, linux-kernel,
Geert Uytterhoeven
The maximum number of micro-TLBs per IPMMU instance is not fixed, but
depends on the SoC type. Hence move it from struct ipmmu_vmsa_device to
struct ipmmu_features, and set up the correct value for both R-Car Gen2
and Gen3 SoCs.
Note that currently no code uses this value.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
drivers/iommu/ipmmu-vmsa.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 4e3134a9a52f8d87..0a21e734466eb1bd 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -42,6 +42,7 @@ struct ipmmu_features {
bool use_ns_alias_offset;
bool has_cache_leaf_nodes;
unsigned int number_of_contexts;
+ unsigned int num_utlbs;
bool setup_imbuscr;
bool twobit_imttbcr_sl0;
bool reserved_context;
@@ -53,7 +54,6 @@ struct ipmmu_vmsa_device {
struct iommu_device iommu;
struct ipmmu_vmsa_device *root;
const struct ipmmu_features *features;
- unsigned int num_utlbs;
unsigned int num_ctx;
spinlock_t lock; /* Protects ctx and domains[] */
DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
@@ -972,6 +972,7 @@ static const struct ipmmu_features ipmmu_features_default = {
.use_ns_alias_offset = true,
.has_cache_leaf_nodes = false,
.number_of_contexts = 1, /* software only tested with one context */
+ .num_utlbs = 32,
.setup_imbuscr = true,
.twobit_imttbcr_sl0 = false,
.reserved_context = false,
@@ -981,6 +982,7 @@ static const struct ipmmu_features ipmmu_features_rcar_gen3 = {
.use_ns_alias_offset = false,
.has_cache_leaf_nodes = true,
.number_of_contexts = 8,
+ .num_utlbs = 48,
.setup_imbuscr = false,
.twobit_imttbcr_sl0 = true,
.reserved_context = true,
@@ -1033,7 +1035,6 @@ static int ipmmu_probe(struct platform_device *pdev)
}
mmu->dev = &pdev->dev;
- mmu->num_utlbs = 48;
spin_lock_init(&mmu->lock);
bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
mmu->features = of_device_get_match_data(&pdev->dev);
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 5/7] iommu/ipmmu-vmsa: Move num_utlbs to SoC-specific features
2019-02-20 15:05 ` [PATCH 5/7] iommu/ipmmu-vmsa: Move num_utlbs to SoC-specific features Geert Uytterhoeven
@ 2019-02-20 15:42 ` Laurent Pinchart
0 siblings, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2019-02-20 15:42 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Joerg Roedel, Magnus Damm, Laurent Pinchart, iommu,
linux-renesas-soc, linux-kernel
Hi Geert,
Thank you for the patch.
On Wed, Feb 20, 2019 at 04:05:29PM +0100, Geert Uytterhoeven wrote:
> The maximum number of micro-TLBs per IPMMU instance is not fixed, but
> depends on the SoC type. Hence move it from struct ipmmu_vmsa_device to
> struct ipmmu_features, and set up the correct value for both R-Car Gen2
> and Gen3 SoCs.
>
> Note that currently no code uses this value.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/iommu/ipmmu-vmsa.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 4e3134a9a52f8d87..0a21e734466eb1bd 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -42,6 +42,7 @@ struct ipmmu_features {
> bool use_ns_alias_offset;
> bool has_cache_leaf_nodes;
> unsigned int number_of_contexts;
> + unsigned int num_utlbs;
> bool setup_imbuscr;
> bool twobit_imttbcr_sl0;
> bool reserved_context;
> @@ -53,7 +54,6 @@ struct ipmmu_vmsa_device {
> struct iommu_device iommu;
> struct ipmmu_vmsa_device *root;
> const struct ipmmu_features *features;
> - unsigned int num_utlbs;
> unsigned int num_ctx;
> spinlock_t lock; /* Protects ctx and domains[] */
> DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> @@ -972,6 +972,7 @@ static const struct ipmmu_features ipmmu_features_default = {
> .use_ns_alias_offset = true,
> .has_cache_leaf_nodes = false,
> .number_of_contexts = 1, /* software only tested with one context */
> + .num_utlbs = 32,
> .setup_imbuscr = true,
> .twobit_imttbcr_sl0 = false,
> .reserved_context = false,
> @@ -981,6 +982,7 @@ static const struct ipmmu_features ipmmu_features_rcar_gen3 = {
> .use_ns_alias_offset = false,
> .has_cache_leaf_nodes = true,
> .number_of_contexts = 8,
> + .num_utlbs = 48,
> .setup_imbuscr = false,
> .twobit_imttbcr_sl0 = true,
> .reserved_context = true,
> @@ -1033,7 +1035,6 @@ static int ipmmu_probe(struct platform_device *pdev)
> }
>
> mmu->dev = &pdev->dev;
> - mmu->num_utlbs = 48;
> spin_lock_init(&mmu->lock);
> bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
> mmu->features = of_device_get_match_data(&pdev->dev);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 6/7] iommu/ipmmu-vmsa: Extract hardware context initialization
2019-02-20 15:05 [PATCH 0/7] iommu/ipmmu-vmsa: Suspend/resume support and assorted cleanups Geert Uytterhoeven
` (4 preceding siblings ...)
2019-02-20 15:05 ` [PATCH 5/7] iommu/ipmmu-vmsa: Move num_utlbs to SoC-specific features Geert Uytterhoeven
@ 2019-02-20 15:05 ` Geert Uytterhoeven
2019-02-20 15:35 ` Laurent Pinchart
2019-02-20 15:05 ` [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support Geert Uytterhoeven
6 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2019-02-20 15:05 UTC (permalink / raw)
To: Joerg Roedel, Magnus Damm
Cc: Laurent Pinchart, iommu, linux-renesas-soc, linux-kernel,
Geert Uytterhoeven
ipmmu_domain_init_context() takes care of (1) initializing the software
domain, and (2) initializing the hardware context for the domain.
Extract the code to initialize the hardware context into a new subroutine
ipmmu_context_init(), to prepare for later reuse.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
drivers/iommu/ipmmu-vmsa.c | 91 ++++++++++++++++++++------------------
1 file changed, 48 insertions(+), 43 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 0a21e734466eb1bd..92a766dd8b459f0c 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -404,52 +404,10 @@ static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
spin_unlock_irqrestore(&mmu->lock, flags);
}
-static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
+static void ipmmu_context_init(struct ipmmu_vmsa_domain *domain)
{
u64 ttbr;
u32 tmp;
- int ret;
-
- /*
- * Allocate the page table operations.
- *
- * VMSA states in section B3.6.3 "Control of Secure or Non-secure memory
- * access, Long-descriptor format" that the NStable bit being set in a
- * table descriptor will result in the NStable and NS bits of all child
- * entries being ignored and considered as being set. The IPMMU seems
- * not to comply with this, as it generates a secure access page fault
- * if any of the NStable and NS bits isn't set when running in
- * non-secure mode.
- */
- domain->cfg.quirks = IO_PGTABLE_QUIRK_ARM_NS;
- domain->cfg.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K;
- domain->cfg.ias = 32;
- domain->cfg.oas = 40;
- domain->cfg.tlb = &ipmmu_gather_ops;
- domain->io_domain.geometry.aperture_end = DMA_BIT_MASK(32);
- domain->io_domain.geometry.force_aperture = true;
- /*
- * TODO: Add support for coherent walk through CCI with DVM and remove
- * cache handling. For now, delegate it to the io-pgtable code.
- */
- domain->cfg.iommu_dev = domain->mmu->root->dev;
-
- /*
- * Find an unused context.
- */
- ret = ipmmu_domain_allocate_context(domain->mmu->root, domain);
- if (ret < 0)
- return ret;
-
- domain->context_id = ret;
-
- domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
- domain);
- if (!domain->iop) {
- ipmmu_domain_free_context(domain->mmu->root,
- domain->context_id);
- return -EINVAL;
- }
/* TTBR0 */
ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
@@ -495,7 +453,54 @@ static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
*/
ipmmu_ctx_write_all(domain, IMCTR,
IMCTR_INTEN | IMCTR_FLUSH | IMCTR_MMUEN);
+}
+
+static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
+{
+ int ret;
+
+ /*
+ * Allocate the page table operations.
+ *
+ * VMSA states in section B3.6.3 "Control of Secure or Non-secure memory
+ * access, Long-descriptor format" that the NStable bit being set in a
+ * table descriptor will result in the NStable and NS bits of all child
+ * entries being ignored and considered as being set. The IPMMU seems
+ * not to comply with this, as it generates a secure access page fault
+ * if any of the NStable and NS bits isn't set when running in
+ * non-secure mode.
+ */
+ domain->cfg.quirks = IO_PGTABLE_QUIRK_ARM_NS;
+ domain->cfg.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K;
+ domain->cfg.ias = 32;
+ domain->cfg.oas = 40;
+ domain->cfg.tlb = &ipmmu_gather_ops;
+ domain->io_domain.geometry.aperture_end = DMA_BIT_MASK(32);
+ domain->io_domain.geometry.force_aperture = true;
+ /*
+ * TODO: Add support for coherent walk through CCI with DVM and remove
+ * cache handling. For now, delegate it to the io-pgtable code.
+ */
+ domain->cfg.iommu_dev = domain->mmu->root->dev;
+
+ /*
+ * Find an unused context.
+ */
+ ret = ipmmu_domain_allocate_context(domain->mmu->root, domain);
+ if (ret < 0)
+ return ret;
+
+ domain->context_id = ret;
+
+ domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
+ domain);
+ if (!domain->iop) {
+ ipmmu_domain_free_context(domain->mmu->root,
+ domain->context_id);
+ return -EINVAL;
+ }
+ ipmmu_context_init(domain);
return 0;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 6/7] iommu/ipmmu-vmsa: Extract hardware context initialization
2019-02-20 15:05 ` [PATCH 6/7] iommu/ipmmu-vmsa: Extract hardware context initialization Geert Uytterhoeven
@ 2019-02-20 15:35 ` Laurent Pinchart
2019-02-20 15:43 ` Geert Uytterhoeven
0 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2019-02-20 15:35 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Joerg Roedel, Magnus Damm, Laurent Pinchart, iommu,
linux-renesas-soc, linux-kernel
Hi Geert,
Thank you for the patch.
On Wed, Feb 20, 2019 at 04:05:30PM +0100, Geert Uytterhoeven wrote:
> ipmmu_domain_init_context() takes care of (1) initializing the software
> domain, and (2) initializing the hardware context for the domain.
>
> Extract the code to initialize the hardware context into a new subroutine
> ipmmu_context_init(), to prepare for later reuse.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> drivers/iommu/ipmmu-vmsa.c | 91 ++++++++++++++++++++------------------
> 1 file changed, 48 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 0a21e734466eb1bd..92a766dd8b459f0c 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -404,52 +404,10 @@ static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
> spin_unlock_irqrestore(&mmu->lock, flags);
> }
>
> -static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
> +static void ipmmu_context_init(struct ipmmu_vmsa_domain *domain)
ipmmu_context_init() vs. ipmmmu_domain_init_context() is confusing. You
could call this one ipmmu_domain_setup_context() maybe ?
> {
> u64 ttbr;
> u32 tmp;
> - int ret;
> -
> - /*
> - * Allocate the page table operations.
> - *
> - * VMSA states in section B3.6.3 "Control of Secure or Non-secure memory
> - * access, Long-descriptor format" that the NStable bit being set in a
> - * table descriptor will result in the NStable and NS bits of all child
> - * entries being ignored and considered as being set. The IPMMU seems
> - * not to comply with this, as it generates a secure access page fault
> - * if any of the NStable and NS bits isn't set when running in
> - * non-secure mode.
> - */
> - domain->cfg.quirks = IO_PGTABLE_QUIRK_ARM_NS;
> - domain->cfg.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K;
> - domain->cfg.ias = 32;
> - domain->cfg.oas = 40;
> - domain->cfg.tlb = &ipmmu_gather_ops;
> - domain->io_domain.geometry.aperture_end = DMA_BIT_MASK(32);
> - domain->io_domain.geometry.force_aperture = true;
> - /*
> - * TODO: Add support for coherent walk through CCI with DVM and remove
> - * cache handling. For now, delegate it to the io-pgtable code.
> - */
> - domain->cfg.iommu_dev = domain->mmu->root->dev;
> -
> - /*
> - * Find an unused context.
> - */
> - ret = ipmmu_domain_allocate_context(domain->mmu->root, domain);
> - if (ret < 0)
> - return ret;
> -
> - domain->context_id = ret;
> -
> - domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
> - domain);
> - if (!domain->iop) {
> - ipmmu_domain_free_context(domain->mmu->root,
> - domain->context_id);
> - return -EINVAL;
> - }
>
> /* TTBR0 */
> ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
> @@ -495,7 +453,54 @@ static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
> */
> ipmmu_ctx_write_all(domain, IMCTR,
> IMCTR_INTEN | IMCTR_FLUSH | IMCTR_MMUEN);
> +}
> +
> +static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
> +{
> + int ret;
> +
> + /*
> + * Allocate the page table operations.
> + *
> + * VMSA states in section B3.6.3 "Control of Secure or Non-secure memory
> + * access, Long-descriptor format" that the NStable bit being set in a
> + * table descriptor will result in the NStable and NS bits of all child
> + * entries being ignored and considered as being set. The IPMMU seems
> + * not to comply with this, as it generates a secure access page fault
> + * if any of the NStable and NS bits isn't set when running in
> + * non-secure mode.
> + */
> + domain->cfg.quirks = IO_PGTABLE_QUIRK_ARM_NS;
> + domain->cfg.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K;
> + domain->cfg.ias = 32;
> + domain->cfg.oas = 40;
> + domain->cfg.tlb = &ipmmu_gather_ops;
> + domain->io_domain.geometry.aperture_end = DMA_BIT_MASK(32);
> + domain->io_domain.geometry.force_aperture = true;
> + /*
> + * TODO: Add support for coherent walk through CCI with DVM and remove
> + * cache handling. For now, delegate it to the io-pgtable code.
> + */
> + domain->cfg.iommu_dev = domain->mmu->root->dev;
> +
> + /*
> + * Find an unused context.
> + */
> + ret = ipmmu_domain_allocate_context(domain->mmu->root, domain);
> + if (ret < 0)
> + return ret;
> +
> + domain->context_id = ret;
> +
> + domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
> + domain);
> + if (!domain->iop) {
> + ipmmu_domain_free_context(domain->mmu->root,
> + domain->context_id);
> + return -EINVAL;
> + }
>
> + ipmmu_context_init(domain);
> return 0;
> }
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 6/7] iommu/ipmmu-vmsa: Extract hardware context initialization
2019-02-20 15:35 ` Laurent Pinchart
@ 2019-02-20 15:43 ` Geert Uytterhoeven
0 siblings, 0 replies; 21+ messages in thread
From: Geert Uytterhoeven @ 2019-02-20 15:43 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Geert Uytterhoeven, Joerg Roedel, Magnus Damm, Laurent Pinchart,
Linux IOMMU, Linux-Renesas, Linux Kernel Mailing List
Hi Laurent,
On Wed, Feb 20, 2019 at 4:35 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Feb 20, 2019 at 04:05:30PM +0100, Geert Uytterhoeven wrote:
> > ipmmu_domain_init_context() takes care of (1) initializing the software
> > domain, and (2) initializing the hardware context for the domain.
> >
> > Extract the code to initialize the hardware context into a new subroutine
> > ipmmu_context_init(), to prepare for later reuse.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > drivers/iommu/ipmmu-vmsa.c | 91 ++++++++++++++++++++------------------
> > 1 file changed, 48 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> > index 0a21e734466eb1bd..92a766dd8b459f0c 100644
> > --- a/drivers/iommu/ipmmu-vmsa.c
> > +++ b/drivers/iommu/ipmmu-vmsa.c
> > @@ -404,52 +404,10 @@ static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
> > spin_unlock_irqrestore(&mmu->lock, flags);
> > }
> >
> > -static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
> > +static void ipmmu_context_init(struct ipmmu_vmsa_domain *domain)
>
> ipmmu_context_init() vs. ipmmmu_domain_init_context() is confusing. You
> could call this one ipmmu_domain_setup_context() maybe ?
Thanks, that name was actually on my shortlist, and may make most sense.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support
2019-02-20 15:05 [PATCH 0/7] iommu/ipmmu-vmsa: Suspend/resume support and assorted cleanups Geert Uytterhoeven
` (5 preceding siblings ...)
2019-02-20 15:05 ` [PATCH 6/7] iommu/ipmmu-vmsa: Extract hardware context initialization Geert Uytterhoeven
@ 2019-02-20 15:05 ` Geert Uytterhoeven
2019-02-20 15:42 ` Laurent Pinchart
2019-02-22 14:29 ` Robin Murphy
6 siblings, 2 replies; 21+ messages in thread
From: Geert Uytterhoeven @ 2019-02-20 15:05 UTC (permalink / raw)
To: Joerg Roedel, Magnus Damm
Cc: Laurent Pinchart, iommu, linux-renesas-soc, linux-kernel,
Geert Uytterhoeven
During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
IPMMU state is lost. Hence after s2ram, devices wired behind an IPMMU,
and configured to use it, will see their DMA operations hang.
To fix this, restore all IPMMU contexts, and re-enable all active
micro-TLBs during system resume.
To avoid overhead on platforms not needing it, the resume code has a
build time dependency on sleep and PSCI support, and a runtime
dependency on PSCI.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
This patch takes a different approach than the BSP, which implements a
bulk save/restore of all registers during system suspend/resume.
drivers/iommu/ipmmu-vmsa.c | 52 +++++++++++++++++++++++++++++++++++++-
1 file changed, 51 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 92a766dd8b459f0c..5d22139914e8f033 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -22,6 +22,7 @@
#include <linux/of_iommu.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
+#include <linux/psci.h>
#include <linux/sizes.h>
#include <linux/slab.h>
#include <linux/sys_soc.h>
@@ -36,7 +37,10 @@
#define arm_iommu_detach_device(...) do {} while (0)
#endif
-#define IPMMU_CTX_MAX 8U
+#define IPMMU_CTX_MAX 8U
+#define IPMMU_CTX_INVALID -1
+
+#define IPMMU_UTLB_MAX 48U
struct ipmmu_features {
bool use_ns_alias_offset;
@@ -58,6 +62,7 @@ struct ipmmu_vmsa_device {
spinlock_t lock; /* Protects ctx and domains[] */
DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
+ s8 utlb_ctx[IPMMU_UTLB_MAX];
struct iommu_group *group;
struct dma_iommu_mapping *mapping;
@@ -335,6 +340,7 @@ static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain *domain,
ipmmu_write(mmu, IMUCTR(utlb),
IMUCTR_TTSEL_MMU(domain->context_id) | IMUCTR_FLUSH |
IMUCTR_MMUEN);
+ mmu->utlb_ctx[utlb] = domain->context_id;
}
/*
@@ -346,6 +352,7 @@ static void ipmmu_utlb_disable(struct ipmmu_vmsa_domain *domain,
struct ipmmu_vmsa_device *mmu = domain->mmu;
ipmmu_write(mmu, IMUCTR(utlb), 0);
+ mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
}
static void ipmmu_tlb_flush_all(void *cookie)
@@ -1043,6 +1050,7 @@ static int ipmmu_probe(struct platform_device *pdev)
spin_lock_init(&mmu->lock);
bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
mmu->features = of_device_get_match_data(&pdev->dev);
+ memset(mmu->utlb_ctx, IPMMU_CTX_INVALID, mmu->features->num_utlbs);
dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
/* Map I/O memory and request IRQ. */
@@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device *pdev)
return 0;
}
+#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
+static int ipmmu_resume_noirq(struct device *dev)
+{
+ struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
+ unsigned int i;
+
+ /* This is the best we can do to check for the presence of PSCI */
+ if (!psci_ops.cpu_suspend)
+ return 0;
+
+ /* Reset root MMU and restore contexts */
+ if (ipmmu_is_root(mmu)) {
+ ipmmu_device_reset(mmu);
+
+ for (i = 0; i < mmu->num_ctx; i++) {
+ if (!mmu->domains[i])
+ continue;
+
+ ipmmu_context_init(mmu->domains[i]);
+ }
+ }
+
+ /* Re-enable active micro-TLBs */
+ for (i = 0; i < mmu->features->num_utlbs; i++) {
+ if (mmu->utlb_ctx[i] == IPMMU_CTX_INVALID)
+ continue;
+
+ ipmmu_utlb_enable(mmu->root->domains[mmu->utlb_ctx[i]], i);
+ }
+
+ return 0;
+}
+
+static const struct dev_pm_ops ipmmu_pm = {
+ SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, ipmmu_resume_noirq)
+};
+#define DEV_PM_OPS &ipmmu_pm
+#else
+#define DEV_PM_OPS NULL
+#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
+
static struct platform_driver ipmmu_driver = {
.driver = {
.name = "ipmmu-vmsa",
.of_match_table = of_match_ptr(ipmmu_of_ids),
+ .pm = DEV_PM_OPS,
},
.probe = ipmmu_probe,
.remove = ipmmu_remove,
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support
2019-02-20 15:05 ` [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support Geert Uytterhoeven
@ 2019-02-20 15:42 ` Laurent Pinchart
2019-02-20 16:05 ` Geert Uytterhoeven
2019-02-22 14:29 ` Robin Murphy
1 sibling, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2019-02-20 15:42 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Joerg Roedel, Magnus Damm, Laurent Pinchart, iommu,
linux-renesas-soc, linux-kernel
Hi Geert,
Thank you for the patch.
On Wed, Feb 20, 2019 at 04:05:31PM +0100, Geert Uytterhoeven wrote:
> During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
> IPMMU state is lost. Hence after s2ram, devices wired behind an IPMMU,
> and configured to use it, will see their DMA operations hang.
>
> To fix this, restore all IPMMU contexts, and re-enable all active
> micro-TLBs during system resume.
>
> To avoid overhead on platforms not needing it, the resume code has a
> build time dependency on sleep and PSCI support, and a runtime
> dependency on PSCI.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> This patch takes a different approach than the BSP, which implements a
> bulk save/restore of all registers during system suspend/resume.
I like this approach better too.
> drivers/iommu/ipmmu-vmsa.c | 52 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 92a766dd8b459f0c..5d22139914e8f033 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -22,6 +22,7 @@
> #include <linux/of_iommu.h>
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> +#include <linux/psci.h>
> #include <linux/sizes.h>
> #include <linux/slab.h>
> #include <linux/sys_soc.h>
> @@ -36,7 +37,10 @@
> #define arm_iommu_detach_device(...) do {} while (0)
> #endif
>
> -#define IPMMU_CTX_MAX 8U
> +#define IPMMU_CTX_MAX 8U
> +#define IPMMU_CTX_INVALID -1
> +
> +#define IPMMU_UTLB_MAX 48U
>
> struct ipmmu_features {
> bool use_ns_alias_offset;
> @@ -58,6 +62,7 @@ struct ipmmu_vmsa_device {
> spinlock_t lock; /* Protects ctx and domains[] */
> DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> + s8 utlb_ctx[IPMMU_UTLB_MAX];
How about making this a bitmask instead to save memory ? I would also
rename it as utlb_ctx doesn't really carry the meaning of the field,
whose purpose is to store whether the µTLB is enabled or disabled.
>
> struct iommu_group *group;
> struct dma_iommu_mapping *mapping;
> @@ -335,6 +340,7 @@ static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain *domain,
> ipmmu_write(mmu, IMUCTR(utlb),
> IMUCTR_TTSEL_MMU(domain->context_id) | IMUCTR_FLUSH |
> IMUCTR_MMUEN);
> + mmu->utlb_ctx[utlb] = domain->context_id;
> }
>
> /*
> @@ -346,6 +352,7 @@ static void ipmmu_utlb_disable(struct ipmmu_vmsa_domain *domain,
> struct ipmmu_vmsa_device *mmu = domain->mmu;
>
> ipmmu_write(mmu, IMUCTR(utlb), 0);
> + mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
> }
>
> static void ipmmu_tlb_flush_all(void *cookie)
> @@ -1043,6 +1050,7 @@ static int ipmmu_probe(struct platform_device *pdev)
> spin_lock_init(&mmu->lock);
> bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
> mmu->features = of_device_get_match_data(&pdev->dev);
> + memset(mmu->utlb_ctx, IPMMU_CTX_INVALID, mmu->features->num_utlbs);
> dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
>
> /* Map I/O memory and request IRQ. */
> @@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
> +static int ipmmu_resume_noirq(struct device *dev)
> +{
> + struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
> + unsigned int i;
> +
> + /* This is the best we can do to check for the presence of PSCI */
> + if (!psci_ops.cpu_suspend)
> + return 0;
PSCI suspend disabling power to the SoC completely may be a common
behaviour on our development boards, but isn't mandated by the PSCI
specification if I'm not mistaken. Is there a way to instead detect that
power has been lost, perhaps by checking whether a register has been
reset to its default value ?
> +
> + /* Reset root MMU and restore contexts */
I think the rest of the code adds a period at the end of sentences in
comments.
> + if (ipmmu_is_root(mmu)) {
> + ipmmu_device_reset(mmu);
> +
> + for (i = 0; i < mmu->num_ctx; i++) {
> + if (!mmu->domains[i])
> + continue;
> +
> + ipmmu_context_init(mmu->domains[i]);
> + }
> + }
> +
> + /* Re-enable active micro-TLBs */
> + for (i = 0; i < mmu->features->num_utlbs; i++) {
> + if (mmu->utlb_ctx[i] == IPMMU_CTX_INVALID)
> + continue;
> +
> + ipmmu_utlb_enable(mmu->root->domains[mmu->utlb_ctx[i]], i);
> + }
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ipmmu_pm = {
> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, ipmmu_resume_noirq)
> +};
> +#define DEV_PM_OPS &ipmmu_pm
> +#else
> +#define DEV_PM_OPS NULL
> +#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
> +
> static struct platform_driver ipmmu_driver = {
> .driver = {
> .name = "ipmmu-vmsa",
> .of_match_table = of_match_ptr(ipmmu_of_ids),
> + .pm = DEV_PM_OPS,
I would have used conditional compilation here instead of using a
DEV_PM_OPS macro, as I think the macro decreases readability (and also
given how its generic name could later conflict with something else).
> },
> .probe = ipmmu_probe,
> .remove = ipmmu_remove,
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support
2019-02-20 15:42 ` Laurent Pinchart
@ 2019-02-20 16:05 ` Geert Uytterhoeven
2019-02-20 16:11 ` Laurent Pinchart
0 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2019-02-20 16:05 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Geert Uytterhoeven, Joerg Roedel, Magnus Damm, Laurent Pinchart,
Linux IOMMU, Linux-Renesas, Linux Kernel Mailing List
Hi Laurent,
On Wed, Feb 20, 2019 at 4:42 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Feb 20, 2019 at 04:05:31PM +0100, Geert Uytterhoeven wrote:
> > During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
> > IPMMU state is lost. Hence after s2ram, devices wired behind an IPMMU,
> > and configured to use it, will see their DMA operations hang.
> >
> > To fix this, restore all IPMMU contexts, and re-enable all active
> > micro-TLBs during system resume.
> >
> > To avoid overhead on platforms not needing it, the resume code has a
> > build time dependency on sleep and PSCI support, and a runtime
> > dependency on PSCI.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > This patch takes a different approach than the BSP, which implements a
> > bulk save/restore of all registers during system suspend/resume.
>
> I like this approach better too.
Thanks ;-)
> > --- a/drivers/iommu/ipmmu-vmsa.c
> > +++ b/drivers/iommu/ipmmu-vmsa.c
> > @@ -58,6 +62,7 @@ struct ipmmu_vmsa_device {
> > spinlock_t lock; /* Protects ctx and domains[] */
> > DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> > struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> > + s8 utlb_ctx[IPMMU_UTLB_MAX];
>
> How about making this a bitmask instead to save memory ? I would also
> rename it as utlb_ctx doesn't really carry the meaning of the field,
> whose purpose is to store whether the µTLB is enabled or disabled.
This field isn't just a binary flag, but stores the context used for the
uTLB, so we can map from micro-TLB to context.
Given there can be 8 contexts, plus the need to indicate unused contexts,
that means 4 bits/micro-TLB. So the overhead is just 24 bytes per IPMMU
instance.
I considered allocating the array dynamically (by having s8 utlb_ctx[]
at the end of the structure), but didn't go that route, as the domains[]
array already uses more memory.
> > @@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > +#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
> > +static int ipmmu_resume_noirq(struct device *dev)
> > +{
> > + struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
> > + unsigned int i;
> > +
> > + /* This is the best we can do to check for the presence of PSCI */
> > + if (!psci_ops.cpu_suspend)
> > + return 0;
>
> PSCI suspend disabling power to the SoC completely may be a common
> behaviour on our development boards, but isn't mandated by the PSCI
> specification if I'm not mistaken. Is there a way to instead detect that
> power has been lost, perhaps by checking whether a register has been
> reset to its default value ?
The approach here is the same as in the clk and pinctrl drivers.
I think we could check if the IMCTR registers for allocated domains in root
IPMMUs are non-zero. But that's about as expensive as doing the full
restore, I think. And it may have to be done for each and every IPMMU
instance, or do you trust caching for this?
> > +
> > + /* Reset root MMU and restore contexts */
>
> I think the rest of the code adds a period at the end of sentences in
> comments.
The balance seems to be just under 50% ;-)
> > + if (ipmmu_is_root(mmu)) {
> > + ipmmu_device_reset(mmu);
> > +
> > + for (i = 0; i < mmu->num_ctx; i++) {
> > + if (!mmu->domains[i])
> > + continue;
> > +
> > + ipmmu_context_init(mmu->domains[i]);
> > + }
> > + }
> > +
> > + /* Re-enable active micro-TLBs */
> > + for (i = 0; i < mmu->features->num_utlbs; i++) {
> > + if (mmu->utlb_ctx[i] == IPMMU_CTX_INVALID)
> > + continue;
> > +
> > + ipmmu_utlb_enable(mmu->root->domains[mmu->utlb_ctx[i]], i);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops ipmmu_pm = {
> > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, ipmmu_resume_noirq)
> > +};
> > +#define DEV_PM_OPS &ipmmu_pm
> > +#else
> > +#define DEV_PM_OPS NULL
> > +#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
> > +
> > static struct platform_driver ipmmu_driver = {
> > .driver = {
> > .name = "ipmmu-vmsa",
> > .of_match_table = of_match_ptr(ipmmu_of_ids),
> > + .pm = DEV_PM_OPS,
>
> I would have used conditional compilation here instead of using a
> DEV_PM_OPS macro, as I think the macro decreases readability (and also
> given how its generic name could later conflict with something else).
You mean
#ifdef ...
.pm = &ipmmu_pm,
#endif
and marking ipmmu_pm __maybe_unused__?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support
2019-02-20 16:05 ` Geert Uytterhoeven
@ 2019-02-20 16:11 ` Laurent Pinchart
2019-02-20 19:47 ` Geert Uytterhoeven
0 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2019-02-20 16:11 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Geert Uytterhoeven, Joerg Roedel, Magnus Damm, Laurent Pinchart,
Linux IOMMU, Linux-Renesas, Linux Kernel Mailing List
Hi Geert,
On Wed, Feb 20, 2019 at 05:05:49PM +0100, Geert Uytterhoeven wrote:
> On Wed, Feb 20, 2019 at 4:42 PM Laurent Pinchart wrote:
> > On Wed, Feb 20, 2019 at 04:05:31PM +0100, Geert Uytterhoeven wrote:
> >> During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
> >> IPMMU state is lost. Hence after s2ram, devices wired behind an IPMMU,
> >> and configured to use it, will see their DMA operations hang.
> >>
> >> To fix this, restore all IPMMU contexts, and re-enable all active
> >> micro-TLBs during system resume.
> >>
> >> To avoid overhead on platforms not needing it, the resume code has a
> >> build time dependency on sleep and PSCI support, and a runtime
> >> dependency on PSCI.
> >>
> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> ---
> >> This patch takes a different approach than the BSP, which implements a
> >> bulk save/restore of all registers during system suspend/resume.
> >
> > I like this approach better too.
>
> Thanks ;-)
>
> >> --- a/drivers/iommu/ipmmu-vmsa.c
> >> +++ b/drivers/iommu/ipmmu-vmsa.c
>
> >> @@ -58,6 +62,7 @@ struct ipmmu_vmsa_device {
> >> spinlock_t lock; /* Protects ctx and domains[] */
> >> DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> >> struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> >> + s8 utlb_ctx[IPMMU_UTLB_MAX];
> >
> > How about making this a bitmask instead to save memory ? I would also
> > rename it as utlb_ctx doesn't really carry the meaning of the field,
> > whose purpose is to store whether the µTLB is enabled or disabled.
>
> This field isn't just a binary flag, but stores the context used for the
> uTLB, so we can map from micro-TLB to context.
> Given there can be 8 contexts, plus the need to indicate unused contexts,
> that means 4 bits/micro-TLB. So the overhead is just 24 bytes per IPMMU
> instance.
My bad, I've overlooked that.
> I considered allocating the array dynamically (by having s8 utlb_ctx[]
> at the end of the structure), but didn't go that route, as the domains[]
> array already uses more memory.
>
> >> @@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device *pdev)
> >> return 0;
> >> }
> >>
> >> +#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
> >> +static int ipmmu_resume_noirq(struct device *dev)
> >> +{
> >> + struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
> >> + unsigned int i;
> >> +
> >> + /* This is the best we can do to check for the presence of PSCI */
> >> + if (!psci_ops.cpu_suspend)
> >> + return 0;
> >
> > PSCI suspend disabling power to the SoC completely may be a common
> > behaviour on our development boards, but isn't mandated by the PSCI
> > specification if I'm not mistaken. Is there a way to instead detect that
> > power has been lost, perhaps by checking whether a register has been
> > reset to its default value ?
>
> The approach here is the same as in the clk and pinctrl drivers.
>
> I think we could check if the IMCTR registers for allocated domains in root
> IPMMUs are non-zero. But that's about as expensive as doing the full
> restore, I think.
Would reading just one register be more expensive that full
reconfiguration ? Or is there no single register that could serve this
purpose ?
> And it may have to be done for each and every IPMMU instance, or do
> you trust caching for this?
If we can find a single register I think that reading it for every IPMMU
instance wouldn't be an issue.
> >> +
> >> + /* Reset root MMU and restore contexts */
> >
> > I think the rest of the code adds a period at the end of sentences in
> > comments.
>
> The balance seems to be just under 50% ;-)
>
> >> + if (ipmmu_is_root(mmu)) {
> >> + ipmmu_device_reset(mmu);
> >> +
> >> + for (i = 0; i < mmu->num_ctx; i++) {
> >> + if (!mmu->domains[i])
> >> + continue;
> >> +
> >> + ipmmu_context_init(mmu->domains[i]);
> >> + }
> >> + }
> >> +
> >> + /* Re-enable active micro-TLBs */
> >> + for (i = 0; i < mmu->features->num_utlbs; i++) {
> >> + if (mmu->utlb_ctx[i] == IPMMU_CTX_INVALID)
> >> + continue;
> >> +
> >> + ipmmu_utlb_enable(mmu->root->domains[mmu->utlb_ctx[i]], i);
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct dev_pm_ops ipmmu_pm = {
> >> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, ipmmu_resume_noirq)
> >> +};
> >> +#define DEV_PM_OPS &ipmmu_pm
> >> +#else
> >> +#define DEV_PM_OPS NULL
> >> +#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
> >> +
> >> static struct platform_driver ipmmu_driver = {
> >> .driver = {
> >> .name = "ipmmu-vmsa",
> >> .of_match_table = of_match_ptr(ipmmu_of_ids),
> >> + .pm = DEV_PM_OPS,
> >
> > I would have used conditional compilation here instead of using a
> > DEV_PM_OPS macro, as I think the macro decreases readability (and also
> > given how its generic name could later conflict with something else).
>
> You mean
>
> #ifdef ...
> .pm = &ipmmu_pm,
> #endif
>
> and marking ipmmu_pm __maybe_unused__?
Yes. Up to you.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support
2019-02-20 16:11 ` Laurent Pinchart
@ 2019-02-20 19:47 ` Geert Uytterhoeven
0 siblings, 0 replies; 21+ messages in thread
From: Geert Uytterhoeven @ 2019-02-20 19:47 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Geert Uytterhoeven, Joerg Roedel, Magnus Damm, Laurent Pinchart,
Linux IOMMU, Linux-Renesas, Linux Kernel Mailing List
Hi Laurent,
On Wed, Feb 20, 2019 at 5:11 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Feb 20, 2019 at 05:05:49PM +0100, Geert Uytterhoeven wrote:
> > On Wed, Feb 20, 2019 at 4:42 PM Laurent Pinchart wrote:
> > > On Wed, Feb 20, 2019 at 04:05:31PM +0100, Geert Uytterhoeven wrote:
> > >> During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
> > >> IPMMU state is lost. Hence after s2ram, devices wired behind an IPMMU,
> > >> and configured to use it, will see their DMA operations hang.
> > >>
> > >> To fix this, restore all IPMMU contexts, and re-enable all active
> > >> micro-TLBs during system resume.
> > >>
> > >> To avoid overhead on platforms not needing it, the resume code has a
> > >> build time dependency on sleep and PSCI support, and a runtime
> > >> dependency on PSCI.
> > >> --- a/drivers/iommu/ipmmu-vmsa.c
> > >> +++ b/drivers/iommu/ipmmu-vmsa.c
> > >> @@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device *pdev)
> > >> return 0;
> > >> }
> > >>
> > >> +#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
> > >> +static int ipmmu_resume_noirq(struct device *dev)
> > >> +{
> > >> + struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
> > >> + unsigned int i;
> > >> +
> > >> + /* This is the best we can do to check for the presence of PSCI */
> > >> + if (!psci_ops.cpu_suspend)
> > >> + return 0;
> > >
> > > PSCI suspend disabling power to the SoC completely may be a common
> > > behaviour on our development boards, but isn't mandated by the PSCI
> > > specification if I'm not mistaken. Is there a way to instead detect that
> > > power has been lost, perhaps by checking whether a register has been
> > > reset to its default value ?
> >
> > The approach here is the same as in the clk and pinctrl drivers.
> >
> > I think we could check if the IMCTR registers for allocated domains in root
> > IPMMUs are non-zero. But that's about as expensive as doing the full
> > restore, I think.
>
> Would reading just one register be more expensive that full
> reconfiguration ? Or is there no single register that could serve this
> purpose ?
>
> > And it may have to be done for each and every IPMMU instance, or do
> > you trust caching for this?
>
> If we can find a single register I think that reading it for every IPMMU
> instance wouldn't be an issue.
Upon more thought, probably it can be done by reading the IMCTR
for the first non-zero domain. Will look into it...
> > >> +static const struct dev_pm_ops ipmmu_pm = {
> > >> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, ipmmu_resume_noirq)
> > >> +};
> > >> +#define DEV_PM_OPS &ipmmu_pm
> > >> +#else
> > >> +#define DEV_PM_OPS NULL
> > >> +#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
> > >> +
> > >> static struct platform_driver ipmmu_driver = {
> > >> .driver = {
> > >> .name = "ipmmu-vmsa",
> > >> .of_match_table = of_match_ptr(ipmmu_of_ids),
> > >> + .pm = DEV_PM_OPS,
> > >
> > > I would have used conditional compilation here instead of using a
> > > DEV_PM_OPS macro, as I think the macro decreases readability (and also
> > > given how its generic name could later conflict with something else).
> >
> > You mean
> >
> > #ifdef ...
> > .pm = &ipmmu_pm,
> > #endif
> >
> > and marking ipmmu_pm __maybe_unused__?
>
> Yes. Up to you.
I'm not such a big fan of __maybe_unused. It's easy to add, and too
easy to forget to remove it when it's no longer needed (casts have the
same problem).
Usually people just annotate the actual suspend/resume methods with
__maybe_unused, which still leaves the (large) struct dev_pm_ops in
memory.
So I started preferring the DEV_PM_OPS approach...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support
2019-02-20 15:05 ` [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support Geert Uytterhoeven
2019-02-20 15:42 ` Laurent Pinchart
@ 2019-02-22 14:29 ` Robin Murphy
1 sibling, 0 replies; 21+ messages in thread
From: Robin Murphy @ 2019-02-22 14:29 UTC (permalink / raw)
To: Geert Uytterhoeven, Joerg Roedel, Magnus Damm
Cc: linux-renesas-soc, iommu, Laurent Pinchart, linux-kernel
Hi Geert,
On 20/02/2019 15:05, Geert Uytterhoeven wrote:
> During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
> IPMMU state is lost. Hence after s2ram, devices wired behind an IPMMU,
> and configured to use it, will see their DMA operations hang.
>
> To fix this, restore all IPMMU contexts, and re-enable all active
> micro-TLBs during system resume.
>
> To avoid overhead on platforms not needing it, the resume code has a
> build time dependency on sleep and PSCI support, and a runtime
> dependency on PSCI.
Frankly, that aspect looks horribly hacky. It's not overly reasonable
for random device drivers to be poking at PSCI internals, and while it
might happen to correlate on some known set of current SoCs with current
firmware, in general it's really too fragile to be accurate:
- Firmware is free to implement suspend callbacks in a way which doesn't
actually lose power.
- Support for CPU_SUSPEND does not imply that SYSTEM_SUSPEND is even
implemented, let alone how it might behave.
- The dev_pm_ops callbacks can also be used for hibernate, wherein it
doesn't really matter what the firmware may or may not do if the user
has pulled the plug and resumed us a week later.
Furthermore, I think any attempt to detect whether you need to resume or
not is inherently fraught with danger - from testing the arm-smmu
runtime PM ops, I've seen register state take a surprisingly long time
to decay in a switched-off power domain, to the point where unless you
check every bit of every register you can't necessarily be certain that
they're really all still valid, and by that point you're doing far more
work than just unconditionally reinitialising the whole state anyway.
Upon resuming from hibernate, the state left by the cold-boot stage
almost certainly *will* be valid, but it probably won't be the state you
actually want.
Really, the whole idea smells of the premature optimisation demons
anyway - is the resume overhead measurably significant?
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> This patch takes a different approach than the BSP, which implements a
> bulk save/restore of all registers during system suspend/resume.
>
> drivers/iommu/ipmmu-vmsa.c | 52 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 92a766dd8b459f0c..5d22139914e8f033 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -22,6 +22,7 @@
> #include <linux/of_iommu.h>
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> +#include <linux/psci.h>
> #include <linux/sizes.h>
> #include <linux/slab.h>
> #include <linux/sys_soc.h>
> @@ -36,7 +37,10 @@
> #define arm_iommu_detach_device(...) do {} while (0)
> #endif
>
> -#define IPMMU_CTX_MAX 8U
> +#define IPMMU_CTX_MAX 8U
> +#define IPMMU_CTX_INVALID -1
> +
> +#define IPMMU_UTLB_MAX 48U
>
> struct ipmmu_features {
> bool use_ns_alias_offset;
> @@ -58,6 +62,7 @@ struct ipmmu_vmsa_device {
> spinlock_t lock; /* Protects ctx and domains[] */
> DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> + s8 utlb_ctx[IPMMU_UTLB_MAX];
>
> struct iommu_group *group;
> struct dma_iommu_mapping *mapping;
> @@ -335,6 +340,7 @@ static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain *domain,
> ipmmu_write(mmu, IMUCTR(utlb),
> IMUCTR_TTSEL_MMU(domain->context_id) | IMUCTR_FLUSH |
> IMUCTR_MMUEN);
> + mmu->utlb_ctx[utlb] = domain->context_id;
> }
>
> /*
> @@ -346,6 +352,7 @@ static void ipmmu_utlb_disable(struct ipmmu_vmsa_domain *domain,
> struct ipmmu_vmsa_device *mmu = domain->mmu;
>
> ipmmu_write(mmu, IMUCTR(utlb), 0);
> + mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
> }
>
> static void ipmmu_tlb_flush_all(void *cookie)
> @@ -1043,6 +1050,7 @@ static int ipmmu_probe(struct platform_device *pdev)
> spin_lock_init(&mmu->lock);
> bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
> mmu->features = of_device_get_match_data(&pdev->dev);
> + memset(mmu->utlb_ctx, IPMMU_CTX_INVALID, mmu->features->num_utlbs);
> dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
>
> /* Map I/O memory and request IRQ. */
> @@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
> +static int ipmmu_resume_noirq(struct device *dev)
> +{
> + struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
> + unsigned int i;
> +
> + /* This is the best we can do to check for the presence of PSCI */
> + if (!psci_ops.cpu_suspend)
> + return 0;
> +
> + /* Reset root MMU and restore contexts */
> + if (ipmmu_is_root(mmu)) {
> + ipmmu_device_reset(mmu);
> +
> + for (i = 0; i < mmu->num_ctx; i++) {
> + if (!mmu->domains[i])
> + continue;
> +
> + ipmmu_context_init(mmu->domains[i]);
> + }
Unless the hardware has some programming sequence requirement, it looks
like it could be a little more efficient to roll this logic into
ipmmu_device_reset() itself such that no IMCR gets written twice, much
like I did with arm_smmu_write_context_bank()/arm-smmu_device_reset().
Robin.
> + }
> +
> + /* Re-enable active micro-TLBs */
> + for (i = 0; i < mmu->features->num_utlbs; i++) {
> + if (mmu->utlb_ctx[i] == IPMMU_CTX_INVALID)
> + continue;
> +
> + ipmmu_utlb_enable(mmu->root->domains[mmu->utlb_ctx[i]], i);
> + }
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ipmmu_pm = {
> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, ipmmu_resume_noirq)
> +};
> +#define DEV_PM_OPS &ipmmu_pm
> +#else
> +#define DEV_PM_OPS NULL
> +#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
> +
> static struct platform_driver ipmmu_driver = {
> .driver = {
> .name = "ipmmu-vmsa",
> .of_match_table = of_match_ptr(ipmmu_of_ids),
> + .pm = DEV_PM_OPS,
> },
> .probe = ipmmu_probe,
> .remove = ipmmu_remove,
>
^ permalink raw reply [flat|nested] 21+ messages in thread