D1 is a RISC-V SoC from Allwinner's sunxi family. This series adds IOMMU
binding and driver support. The RISC-V architecture code still needs
some small updates to use an IOMMU for DMA[1][2]. I will send those
separately.
[1]: https://lore.kernel.org/linux-riscv/[email protected]/
[2]: https://lore.kernel.org/linux-riscv/[email protected]/
Changes in v2:
- Disallow the 'resets' property for the D1 variant
- Set bypass based on attached devices instead of using a fixed value
Samuel Holland (6):
dt-bindings: iommu: sun50i: Add compatible for Allwinner D1
iommu/sun50i: Track masters attached to the domain
iommu/sun50i: Keep the bypass register up to date
iommu/sun50i: Support variants without an external reset
iommu/sun50i: Add support for the D1 variant
riscv: dts: allwinner: d1: Add the IOMMU node
.../iommu/allwinner,sun50i-h6-iommu.yaml | 20 +++++-
.../boot/dts/allwinner/sunxi-d1s-t113.dtsi | 10 +++
drivers/iommu/sun50i-iommu.c | 68 ++++++++++++++-----
3 files changed, 79 insertions(+), 19 deletions(-)
--
2.37.4
Currently, the IOMMU driver leaves the bypass register at its default
value. The H6 variant of the hardware disables bypass by default. So
once the first device is attached to the IOMMU, translation is enabled
for all masters, even those not attached to an IOMMU group/domain.
On the other hand, the D1 hardware variant enables bypass by default, so
keeping the default value prevents the IOMMU from functioning entirely.
Fixes: 4100b8c229b3 ("iommu: Add Allwinner H6 IOMMU driver")
Signed-off-by: Samuel Holland <[email protected]>
---
Changes in v2:
- Set bypass based on attached devices instead of using a fixed value
drivers/iommu/sun50i-iommu.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 3757d5a18318..a3a462933c62 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -441,6 +441,9 @@ static int sun50i_iommu_enable(struct sun50i_iommu *iommu)
spin_lock_irqsave(&iommu->iommu_lock, flags);
+ iommu_write(iommu, IOMMU_BYPASS_REG,
+ ~atomic_read(&sun50i_domain->masters));
+
iommu_write(iommu, IOMMU_TTB_REG, sun50i_domain->dt_dma);
iommu_write(iommu, IOMMU_TLB_PREFETCH_REG,
IOMMU_TLB_PREFETCH_MASTER_ENABLE(0) |
@@ -755,6 +758,17 @@ static void sun50i_iommu_detach_domain(struct sun50i_iommu *iommu,
iommu->domain = NULL;
}
+static void sun50i_iommu_update_masters(struct sun50i_iommu *iommu,
+ struct sun50i_iommu_domain *sun50i_domain)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&iommu->iommu_lock, flags);
+ iommu_write(iommu, IOMMU_BYPASS_REG,
+ ~atomic_read(&sun50i_domain->masters));
+ spin_unlock_irqrestore(&iommu->iommu_lock, flags);
+}
+
static void sun50i_iommu_detach_device(struct iommu_domain *domain,
struct device *dev)
{
@@ -770,6 +784,8 @@ static void sun50i_iommu_detach_device(struct iommu_domain *domain,
if (atomic_fetch_andnot(masters, &sun50i_domain->masters) == masters)
sun50i_iommu_detach_domain(iommu, sun50i_domain);
+ else
+ sun50i_iommu_update_masters(iommu, sun50i_domain);
}
static int sun50i_iommu_attach_device(struct iommu_domain *domain,
@@ -791,6 +807,8 @@ static int sun50i_iommu_attach_device(struct iommu_domain *domain,
if (atomic_fetch_or(masters, &sun50i_domain->masters) == 0)
sun50i_iommu_attach_domain(iommu, sun50i_domain);
+ else
+ sun50i_iommu_update_masters(iommu, sun50i_domain);
return 0;
}
--
2.37.4
D1 contains an IOMMU similar to the one in the H6 SoC, but the D1
variant has no external reset signal.
Signed-off-by: Samuel Holland <[email protected]>
---
Changes in v2:
- Disallow the 'resets' property for the D1 variant
.../iommu/allwinner,sun50i-h6-iommu.yaml | 20 +++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/iommu/allwinner,sun50i-h6-iommu.yaml b/Documentation/devicetree/bindings/iommu/allwinner,sun50i-h6-iommu.yaml
index e20016f12017..5aeea10cf899 100644
--- a/Documentation/devicetree/bindings/iommu/allwinner,sun50i-h6-iommu.yaml
+++ b/Documentation/devicetree/bindings/iommu/allwinner,sun50i-h6-iommu.yaml
@@ -17,7 +17,9 @@ properties:
The content of the cell is the master ID.
compatible:
- const: allwinner,sun50i-h6-iommu
+ enum:
+ - allwinner,sun20i-d1-iommu
+ - allwinner,sun50i-h6-iommu
reg:
maxItems: 1
@@ -37,7 +39,21 @@ required:
- reg
- interrupts
- clocks
- - resets
+
+if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - allwinner,sun50i-h6-iommu
+
+then:
+ required:
+ - resets
+
+else:
+ properties:
+ resets: false
additionalProperties: false
--
2.37.4
The refcount logic here is broken. The refcount is initialized to 1 with
no devices attached, so it will be decremented back to 1 when the last
device is detached. However, refcount_dec_and_test() returns true only
when the refcount gets decremented to zero, so the domain will never be
cleaned up, and the hardware will never be disabled.
Replace the refcount with a mask of masters attached to the domain. The
domain can be cleaned up when the last master detaches. This mask is
also necessary to correctly set the bypass register.
Fixes: 4100b8c229b3 ("iommu: Add Allwinner H6 IOMMU driver")
Signed-off-by: Samuel Holland <[email protected]>
---
Changes in v2:
- New patch for v2
drivers/iommu/sun50i-iommu.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 5b585eace3d4..3757d5a18318 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -114,8 +114,8 @@ struct sun50i_iommu {
struct sun50i_iommu_domain {
struct iommu_domain domain;
- /* Number of devices attached to the domain */
- refcount_t refcnt;
+ /* Mask of masters attached to this domain */
+ atomic_t masters;
/* L1 Page Table */
u32 *dt;
@@ -684,8 +684,6 @@ static struct iommu_domain *sun50i_iommu_domain_alloc(unsigned type)
if (!sun50i_domain->dt)
goto err_free_domain;
- refcount_set(&sun50i_domain->refcnt, 1);
-
sun50i_domain->domain.geometry.aperture_start = 0;
sun50i_domain->domain.geometry.aperture_end = DMA_BIT_MASK(32);
sun50i_domain->domain.geometry.force_aperture = true;
@@ -760,23 +758,27 @@ static void sun50i_iommu_detach_domain(struct sun50i_iommu *iommu,
static void sun50i_iommu_detach_device(struct iommu_domain *domain,
struct device *dev)
{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct sun50i_iommu_domain *sun50i_domain = to_sun50i_domain(domain);
struct sun50i_iommu *iommu = dev_iommu_priv_get(dev);
+ int masters = 0;
dev_dbg(dev, "Detaching from IOMMU domain\n");
- if (iommu->domain != domain)
- return;
+ for (unsigned int i = 0; i < fwspec->num_ids; i++)
+ masters |= BIT(fwspec->ids[i]);
- if (refcount_dec_and_test(&sun50i_domain->refcnt))
+ if (atomic_fetch_andnot(masters, &sun50i_domain->masters) == masters)
sun50i_iommu_detach_domain(iommu, sun50i_domain);
}
static int sun50i_iommu_attach_device(struct iommu_domain *domain,
struct device *dev)
{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct sun50i_iommu_domain *sun50i_domain = to_sun50i_domain(domain);
struct sun50i_iommu *iommu;
+ int masters = 0;
iommu = sun50i_iommu_from_dev(dev);
if (!iommu)
@@ -784,15 +786,11 @@ static int sun50i_iommu_attach_device(struct iommu_domain *domain,
dev_dbg(dev, "Attaching to IOMMU domain\n");
- refcount_inc(&sun50i_domain->refcnt);
-
- if (iommu->domain == domain)
- return 0;
-
- if (iommu->domain)
- sun50i_iommu_detach_device(iommu->domain, dev);
+ for (unsigned int i = 0; i < fwspec->num_ids; i++)
+ masters |= BIT(fwspec->ids[i]);
- sun50i_iommu_attach_domain(iommu, sun50i_domain);
+ if (atomic_fetch_or(masters, &sun50i_domain->masters) == 0)
+ sun50i_iommu_attach_domain(iommu, sun50i_domain);
return 0;
}
--
2.37.4
The IOMMU in the Allwinner D1 SoC does not have an external reset line.
Only attempt to get the reset on hardware variants which should have one
according to the binding. And switch from the deprecated function to the
explicit "exclusive" variant.
Reviewed-by: Jernej Skrabec <[email protected]>
Reviewed-by: Philipp Zabel <[email protected]>
Signed-off-by: Samuel Holland <[email protected]>
---
(no changes since v1)
drivers/iommu/sun50i-iommu.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index a3a462933c62..d19f6ce25f76 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -95,6 +95,10 @@
#define SPAGE_SIZE 4096
+struct sun50i_iommu_variant {
+ bool has_reset;
+};
+
struct sun50i_iommu {
struct iommu_device iommu;
@@ -995,9 +999,14 @@ static irqreturn_t sun50i_iommu_irq(int irq, void *dev_id)
static int sun50i_iommu_probe(struct platform_device *pdev)
{
+ const struct sun50i_iommu_variant *variant;
struct sun50i_iommu *iommu;
int ret, irq;
+ variant = of_device_get_match_data(&pdev->dev);
+ if (!variant)
+ return -EINVAL;
+
iommu = devm_kzalloc(&pdev->dev, sizeof(*iommu), GFP_KERNEL);
if (!iommu)
return -ENOMEM;
@@ -1037,7 +1046,8 @@ static int sun50i_iommu_probe(struct platform_device *pdev)
goto err_free_group;
}
- iommu->reset = devm_reset_control_get(&pdev->dev, NULL);
+ if (variant->has_reset)
+ iommu->reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
if (IS_ERR(iommu->reset)) {
dev_err(&pdev->dev, "Couldn't get our reset line.\n");
ret = PTR_ERR(iommu->reset);
@@ -1075,8 +1085,12 @@ static int sun50i_iommu_probe(struct platform_device *pdev)
return ret;
}
+static const struct sun50i_iommu_variant sun50i_h6_iommu = {
+ .has_reset = true,
+};
+
static const struct of_device_id sun50i_iommu_dt[] = {
- { .compatible = "allwinner,sun50i-h6-iommu", },
+ { .compatible = "allwinner,sun50i-h6-iommu", .data = &sun50i_h6_iommu },
{ /* sentinel */ },
};
MODULE_DEVICE_TABLE(of, sun50i_iommu_dt);
--
2.37.4
Dne torek, 03. januar 2023 ob 02:08:59 CET je Samuel Holland napisal(a):
> The refcount logic here is broken. The refcount is initialized to 1 with
> no devices attached, so it will be decremented back to 1 when the last
> device is detached. However, refcount_dec_and_test() returns true only
> when the refcount gets decremented to zero, so the domain will never be
> cleaned up, and the hardware will never be disabled.
>
> Replace the refcount with a mask of masters attached to the domain. The
> domain can be cleaned up when the last master detaches. This mask is
> also necessary to correctly set the bypass register.
>
> Fixes: 4100b8c229b3 ("iommu: Add Allwinner H6 IOMMU driver")
> Signed-off-by: Samuel Holland <[email protected]>
Reviewed-by: Jernej Skrabec <[email protected]>
Best regards,
Jernej
Dne torek, 03. januar 2023 ob 02:09:00 CET je Samuel Holland napisal(a):
> Currently, the IOMMU driver leaves the bypass register at its default
> value. The H6 variant of the hardware disables bypass by default. So
> once the first device is attached to the IOMMU, translation is enabled
> for all masters, even those not attached to an IOMMU group/domain.
>
> On the other hand, the D1 hardware variant enables bypass by default, so
> keeping the default value prevents the IOMMU from functioning entirely.
>
> Fixes: 4100b8c229b3 ("iommu: Add Allwinner H6 IOMMU driver")
> Signed-off-by: Samuel Holland <[email protected]>
Reviewed-by: Jernej Skrabec <[email protected]>
Best regards,
Jernej
On Mon, 02 Jan 2023 19:08:58 -0600, Samuel Holland wrote:
> D1 contains an IOMMU similar to the one in the H6 SoC, but the D1
> variant has no external reset signal.
>
> Signed-off-by: Samuel Holland <[email protected]>
> ---
>
> Changes in v2:
> - Disallow the 'resets' property for the D1 variant
>
> .../iommu/allwinner,sun50i-h6-iommu.yaml | 20 +++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
Reviewed-by: Rob Herring <[email protected]>
Hi Samuel,
On Mon, Jan 02, 2023 at 07:08:57PM -0600, Samuel Holland wrote:
> Samuel Holland (6):
> dt-bindings: iommu: sun50i: Add compatible for Allwinner D1
> iommu/sun50i: Track masters attached to the domain
> iommu/sun50i: Keep the bypass register up to date
> iommu/sun50i: Support variants without an external reset
> iommu/sun50i: Add support for the D1 variant
> riscv: dts: allwinner: d1: Add the IOMMU node
There is a conflict between these patches and changes in the IOMMU core
branch. With those merged in there is a compile warning about
sun50i_iommu_detach_domain() being unused. Fixing that requires removing
of 3-4 functions, which I am not sure is the right solution.
I pushed the arm/allwinner branch out to the iommu tree (with the core
branch merged in) and exluded it from next for now. Can you please have
a look and fix this up in a way that does not break the driver?
Once this is fixed I will include the arm/allwinner branch into my next
branch again.
Thanks,
Joerg
Hi Samuel,
On Fri, Jan 20, 2023 at 04:11:30PM +0100, Joerg Roedel wrote:
> Once this is fixed I will include the arm/allwinner branch into my next
> branch again.
Since there was no reply to this I nuked the patches from the IOMMU
tree. If this is still relevant please resubmit them after the next
merge window.
Regards,
Joerg
Hi Joerg,
On 2/3/23 04:21, Joerg Roedel wrote:
> On Fri, Jan 20, 2023 at 04:11:30PM +0100, Joerg Roedel wrote:
>> There is a conflict between these patches and changes in the IOMMU
>> core branch. With those merged in there is a compile warning about
>> sun50i_iommu_detach_domain() being unused. Fixing that requires
>> removing of 3-4 functions, which I am not sure is the right
>> solution.
>>
>> Once this is fixed I will include the arm/allwinner branch into my
>> next branch again.
>
> Since there was no reply to this I nuked the patches from the IOMMU
> tree. If this is still relevant please resubmit them after the next
> merge window.
I am not sure what the right solution is here either, and I have not had
the chance to look at it again. With my limited understanding of how the
default domain logic works, and the fact that the IOMMU driver only
supports one domain, it seems the driver should keep that domain enabled
permanently, regardless of which devices are attached. So my patch 2
would be wrong. I will investigate and send a v3 after the merge window.
Regards,
Samuel