[PATCH] iommu/rockchip: Add flush_iotlb_all ops

[PATCH] iommu/rockchip: Add flush_iotlb_all ops

[Detlev Casanova]

From: Jonas Karlman <jonas@kwiboo.se>

On some Rockchip cores (like the vdpu34x video decoder), the IOMMU device
is inside the the device that uses it.

The IOMMU device can still be driven by the iommu driver, but when an
error occurs in the main device (e.g. a decoding error that resets the
decoder), the IOMMU device will also be reseted.
In such situation, the IOMMU driver and the hardware are out of sync and
IOMMU errors will start popping up.

To avoid that, add a flush_iotlb_all function that will let the main drivers
(e.g. rkvdec) tell the IOMMU driver to write all its cached mappings into
the IOMMU hardware when such an error occured.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 drivers/iommu/rockchip-iommu.c | 45 ++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 323cc665c357..7086716cb8fc 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -899,6 +899,40 @@ static size_t rk_iommu_unmap(struct iommu_domain *domain, unsigned long _iova,
 	return unmap_size;
 }
 
+static void rk_iommu_flush_iotlb_all(struct iommu_domain *domain)
+{
+	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
+	struct list_head *pos;
+	unsigned long flags;
+	int i, ret;
+
+	spin_lock_irqsave(&rk_domain->iommus_lock, flags);
+	list_for_each(pos, &rk_domain->iommus) {
+		struct rk_iommu *iommu = list_entry(pos, struct rk_iommu, node);
+
+		ret = pm_runtime_get_if_in_use(iommu->dev);
+		if (!ret || WARN_ON_ONCE(ret < 0))
+			continue;
+
+		if (WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)))
+			continue;
+
+		rk_iommu_enable_stall(iommu);
+		for (i = 0; i < iommu->num_mmu; i++) {
+			rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR,
+				rk_ops->mk_dtentries(rk_domain->dt_dma));
+			rk_iommu_base_command(iommu->bases[i], RK_MMU_CMD_ZAP_CACHE);
+			rk_iommu_write(iommu->bases[i], RK_MMU_INT_MASK, RK_MMU_IRQ_MASK);
+		}
+		rk_iommu_enable_paging(iommu);
+		rk_iommu_disable_stall(iommu);
+
+		clk_bulk_disable(iommu->num_clocks, iommu->clocks);
+		pm_runtime_put(iommu->dev);
+	}
+	spin_unlock_irqrestore(&rk_domain->iommus_lock, flags);
+}
+
 static struct rk_iommu *rk_iommu_from_dev(struct device *dev)
 {
 	struct rk_iommudata *data = dev_iommu_priv_get(dev);
@@ -1172,11 +1206,12 @@ static const struct iommu_ops rk_iommu_ops = {
 	.pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP,
 	.of_xlate = rk_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev	= rk_iommu_attach_device,
-		.map_pages	= rk_iommu_map,
-		.unmap_pages	= rk_iommu_unmap,
-		.iova_to_phys	= rk_iommu_iova_to_phys,
-		.free		= rk_iommu_domain_free,
+		.attach_dev		= rk_iommu_attach_device,
+		.map_pages		= rk_iommu_map,
+		.unmap_pages		= rk_iommu_unmap,
+		.flush_iotlb_all	= rk_iommu_flush_iotlb_all,
+		.iova_to_phys		= rk_iommu_iova_to_phys,
+		.free			= rk_iommu_domain_free,
 	}
 };
 
-- 
2.49.0

Re: [PATCH] iommu/rockchip: Add flush_iotlb_all ops

[Robin Murphy]

On 18/03/2025 3:20 pm, Detlev Casanova wrote:
> From: Jonas Karlman <jonas@kwiboo.se>
> 
> On some Rockchip cores (like the vdpu34x video decoder), the IOMMU device
> is inside the the device that uses it.
> 
> The IOMMU device can still be driven by the iommu driver, but when an
> error occurs in the main device (e.g. a decoding error that resets the
> decoder), the IOMMU device will also be reseted.
> In such situation, the IOMMU driver and the hardware are out of sync and
> IOMMU errors will start popping up.
> 
> To avoid that, add a flush_iotlb_all function that will let the main drivers
> (e.g. rkvdec) tell the IOMMU driver to write all its cached mappings into
> the IOMMU hardware when such an error occured.

Eww, this is the exact opposite of what flush_iotlb_all represents, and 
I really don't like the idea of the public IOMMU API being abused for 
inter-driver communication. Please have some kind of proper reset 
notifier mechanism - in fact with runtime PM could you not already 
invoke a suspend/resume cycle via the device links? AFAICS it would also 
work to attach to a different domain then switch back again. Or at worst 
just export a public interface for the other driver to invoke 
rk_iommu_resume() directly. Just don't hide it in something completely 
inappropriate - I mean, consider if someone wants to implement 
IOMMU_CAP_DEFERRED_FLUSH support here in future...

Thanks,
Robin.

> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>   drivers/iommu/rockchip-iommu.c | 45 ++++++++++++++++++++++++++++++----
>   1 file changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 323cc665c357..7086716cb8fc 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -899,6 +899,40 @@ static size_t rk_iommu_unmap(struct iommu_domain *domain, unsigned long _iova,
>   	return unmap_size;
>   }
>   
> +static void rk_iommu_flush_iotlb_all(struct iommu_domain *domain)
> +{
> +	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
> +	struct list_head *pos;
> +	unsigned long flags;
> +	int i, ret;
> +
> +	spin_lock_irqsave(&rk_domain->iommus_lock, flags);
> +	list_for_each(pos, &rk_domain->iommus) {
> +		struct rk_iommu *iommu = list_entry(pos, struct rk_iommu, node);
> +
> +		ret = pm_runtime_get_if_in_use(iommu->dev);
> +		if (!ret || WARN_ON_ONCE(ret < 0))
> +			continue;
> +
> +		if (WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)))
> +			continue;
> +
> +		rk_iommu_enable_stall(iommu);
> +		for (i = 0; i < iommu->num_mmu; i++) {
> +			rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR,
> +				rk_ops->mk_dtentries(rk_domain->dt_dma));
> +			rk_iommu_base_command(iommu->bases[i], RK_MMU_CMD_ZAP_CACHE);
> +			rk_iommu_write(iommu->bases[i], RK_MMU_INT_MASK, RK_MMU_IRQ_MASK);
> +		}
> +		rk_iommu_enable_paging(iommu);
> +		rk_iommu_disable_stall(iommu);
> +
> +		clk_bulk_disable(iommu->num_clocks, iommu->clocks);
> +		pm_runtime_put(iommu->dev);
> +	}
> +	spin_unlock_irqrestore(&rk_domain->iommus_lock, flags);
> +}
> +
>   static struct rk_iommu *rk_iommu_from_dev(struct device *dev)
>   {
>   	struct rk_iommudata *data = dev_iommu_priv_get(dev);
> @@ -1172,11 +1206,12 @@ static const struct iommu_ops rk_iommu_ops = {
>   	.pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP,
>   	.of_xlate = rk_iommu_of_xlate,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= rk_iommu_attach_device,
> -		.map_pages	= rk_iommu_map,
> -		.unmap_pages	= rk_iommu_unmap,
> -		.iova_to_phys	= rk_iommu_iova_to_phys,
> -		.free		= rk_iommu_domain_free,
> +		.attach_dev		= rk_iommu_attach_device,
> +		.map_pages		= rk_iommu_map,
> +		.unmap_pages		= rk_iommu_unmap,
> +		.flush_iotlb_all	= rk_iommu_flush_iotlb_all,
> +		.iova_to_phys		= rk_iommu_iova_to_phys,
> +		.free			= rk_iommu_domain_free,
>   	}
>   };
>   

Re: [PATCH] iommu/rockchip: Add flush_iotlb_all ops

[Detlev Casanova]

Hi Robin,
On Tuesday, 18 March 2025 14:40:20 EDT Robin Murphy wrote:
> On 18/03/2025 3:20 pm, Detlev Casanova wrote:
> > From: Jonas Karlman <jonas@kwiboo.se>
> > 
> > On some Rockchip cores (like the vdpu34x video decoder), the IOMMU device
> > is inside the the device that uses it.
> > 
> > The IOMMU device can still be driven by the iommu driver, but when an
> > error occurs in the main device (e.g. a decoding error that resets the
> > decoder), the IOMMU device will also be reseted.
> > In such situation, the IOMMU driver and the hardware are out of sync and
> > IOMMU errors will start popping up.
> > 
> > To avoid that, add a flush_iotlb_all function that will let the main
> > drivers (e.g. rkvdec) tell the IOMMU driver to write all its cached
> > mappings into the IOMMU hardware when such an error occured.
> 
> Eww, this is the exact opposite of what flush_iotlb_all represents, and
> I really don't like the idea of the public IOMMU API being abused for
> inter-driver communication.

I see, so "Synchronously flush all hardware TLBs for this domain" means that it 
is supposed to clear everything that is set in hardware then, right ?

> Please have some kind of proper reset
> notifier mechanism - in fact with runtime PM could you not already
> invoke a suspend/resume cycle via the device links? AFAICS it would also
> work to attach to a different domain then switch back again. Or at worst
> just export a public interface for the other driver to invoke
> rk_iommu_resume() directly.

Thank you for the suggestions. PM runtime is a bit overkill as it can also 
manage power and clocks related to the iommu but also other devices in the 
tree.

I went with an "empty domain" and switching between them works as expected.

> Just don't hide it in something completely
> inappropriate - I mean, consider if someone wants to implement
> IOMMU_CAP_DEFERRED_FLUSH support here in future...

I don't know what that does exactly. As I don't have enough of the picture on 
IOMMU drivers, I'm glad we could find a solution that doesn't change anything 
in the IOMMU tree :)

Regards,
Detlev

> 
> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >   drivers/iommu/rockchip-iommu.c | 45 ++++++++++++++++++++++++++++++----
> >   1 file changed, 40 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iommu/rockchip-iommu.c
> > b/drivers/iommu/rockchip-iommu.c index 323cc665c357..7086716cb8fc 100644
> > --- a/drivers/iommu/rockchip-iommu.c
> > +++ b/drivers/iommu/rockchip-iommu.c
> > @@ -899,6 +899,40 @@ static size_t rk_iommu_unmap(struct iommu_domain
> > *domain, unsigned long _iova,> 
> >   	return unmap_size;
> >   
> >   }
> > 
> > +static void rk_iommu_flush_iotlb_all(struct iommu_domain *domain)
> > +{
> > +	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
> > +	struct list_head *pos;
> > +	unsigned long flags;
> > +	int i, ret;
> > +
> > +	spin_lock_irqsave(&rk_domain->iommus_lock, flags);
> > +	list_for_each(pos, &rk_domain->iommus) {
> > +		struct rk_iommu *iommu = list_entry(pos, struct 
rk_iommu, node);
> > +
> > +		ret = pm_runtime_get_if_in_use(iommu->dev);
> > +		if (!ret || WARN_ON_ONCE(ret < 0))
> > +			continue;
> > +
> > +		if (WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu-
>clocks)))
> > +			continue;
> > +
> > +		rk_iommu_enable_stall(iommu);
> > +		for (i = 0; i < iommu->num_mmu; i++) {
> > +			rk_iommu_write(iommu->bases[i], 
RK_MMU_DTE_ADDR,
> > +				rk_ops->mk_dtentries(rk_domain-
>dt_dma));
> > +			rk_iommu_base_command(iommu->bases[i], 
RK_MMU_CMD_ZAP_CACHE);
> > +			rk_iommu_write(iommu->bases[i], 
RK_MMU_INT_MASK, RK_MMU_IRQ_MASK);
> > +		}
> > +		rk_iommu_enable_paging(iommu);
> > +		rk_iommu_disable_stall(iommu);
> > +
> > +		clk_bulk_disable(iommu->num_clocks, iommu->clocks);
> > +		pm_runtime_put(iommu->dev);
> > +	}
> > +	spin_unlock_irqrestore(&rk_domain->iommus_lock, flags);
> > +}
> > +
> > 
> >   static struct rk_iommu *rk_iommu_from_dev(struct device *dev)
> >   {
> >   
> >   	struct rk_iommudata *data = dev_iommu_priv_get(dev);
> > 
> > @@ -1172,11 +1206,12 @@ static const struct iommu_ops rk_iommu_ops = {
> > 
> >   	.pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP,
> >   	.of_xlate = rk_iommu_of_xlate,
> >   	.default_domain_ops = &(const struct iommu_domain_ops) {
> > 
> > -		.attach_dev	= rk_iommu_attach_device,
> > -		.map_pages	= rk_iommu_map,
> > -		.unmap_pages	= rk_iommu_unmap,
> > -		.iova_to_phys	= rk_iommu_iova_to_phys,
> > -		.free		= rk_iommu_domain_free,
> > +		.attach_dev		= rk_iommu_attach_device,
> > +		.map_pages		= rk_iommu_map,
> > +		.unmap_pages		= rk_iommu_unmap,
> > +		.flush_iotlb_all	= rk_iommu_flush_iotlb_all,
> > +		.iova_to_phys		= 
rk_iommu_iova_to_phys,
> > +		.free			= 
rk_iommu_domain_free,
> > 
> >   	}
> >   
> >   };