Re: [PATCH 3/3] iommu: apple-dart: Add 4-level page table support

[Sven Peter <sven@kernel.org>]

On 16.08.25 16:19, Janne Grunau wrote:
> Hej,
> 
> On Sat, Aug 16, 2025, at 15:50, Sven Peter wrote:
>> On 14.08.25 10:40, Janne Grunau wrote:
>>> From: Hector Martin <marcan@marcan.st>
>>>
>>> The T8110 variant DART implementation on T602x SoCs indicates an IAS
>>> of 42, which requires an extra page table level. The extra level is
>>> optional, but let's implement it.
>>>
>>> Since the driver failed at IO page table creation with 42-bit IAS add
>>> "apple,t6020-dart" as separate compatible using the T8110 HW data.
>>
>> Is the commit description outdated? I don't see this change anywhere.
> 
> yes, I decided to handle this as missing feature / bug. Both end up with
> the same result and as far as we can tell it is fully compatible.
> Removed locally.
> 
>>> Later it might be useful to restrict this based on the actual
>>> attached devices, since most won't need that much address space
>>> anyway.
>>>
>>> Signed-off-by: Hector Martin <marcan@marcan.st> Signed-off-by: Janne
>>> Grunau <j@jannau.net>
>>> ---
>>>    drivers/iommu/apple-dart.c | 23 +++++++++++++++++------ 1 file
>>>    changed, 17 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
>>> index e72a93e78e26ca61b233c83d439dbdfadf040fc6..bb48e8603d6c84bcf107-
>>> 294d851c2f2fc1273298 100644 --- a/drivers/iommu/apple-dart.c +++ b/drivers/iommu/apple-
>>> dart.c @@ -133,6 +133,7 @@  #define DART_T8110_TCR
>>> 0x1000  #define DART_T8110_TCR_REMAP            GENMASK(11, 8)
>>> #define DART_T8110_TCR_REMAP_EN         BIT(7) +#define
>>> DART_T8110_TCR_FOUR_LEVEL       BIT(3)  #define
>>> DART_T8110_TCR_BYPASS_DAPF      BIT(2)  #define
>>> DART_T8110_TCR_BYPASS_DART      BIT(1)  #define
>>> DART_T8110_TCR_TRANSLATE_ENABLE BIT(0) @@ -177,6 +178,7 @@ struct
>>> apple_dart_hw {     u32 tcr_enabled;     u32 tcr_disabled;     u32
>>> tcr_bypass;
>>> +    u32 tcr_4level;
>>>
>>>       u32 ttbr; u32 ttbr_valid; @@ -217,6 +219,7 @@ struct apple_dart
>>>       { u32 pgsize; u32 num_streams; u32 supports_bypass : 1;
>>> +    u32 four_level : 1;
>>>
>>>       struct iommu_group *sid2group[DART_MAX_STREAMS]; struct
>>>       iommu_device iommu; @@ -305,13 +308,16 @@ static struct
>>>       apple_dart_domain *to_dart_domain(struct iommu_domain *dom) }
>>>
>>>    static void -apple_dart_hw_enable_translation(struct
>>>    apple_dart_stream_map *stream_map)
>>>    +apple_dart_hw_enable_translation(struct apple_dart_stream_map
>>>    *stream_map, int levels) {   struct apple_dart *dart = stream_map-
>>>    >dart;   int sid;
>>>
>>> +    WARN_ON(levels != 3 && levels != 4);
>>> +    WARN_ON(levels == 4 && !dart->four_level); for_each_set_bit(sid,
>>>       stream_map->sidmap, dart->num_streams)
>>> -            writel(dart->hw->tcr_enabled, dart->regs +
>>>               DART_TCR(dart, sid));
>>> +            writel(dart->hw->tcr_enabled | (levels == 4 ? dart->hw-
>>>               >tcr_4level : 0),
>>> +                   dart->regs + DART_TCR(dart, sid));
>>
>> This is a bit hard to read, I'd prefer an explicit if (dart->hw-
>>> tcr_4level) here.
> 
> you mean `if (levels == 4)`? `dart->hw->tcr_4level` will be `BIT(3)` for
> t8110 darts even when they use just 3 page table levels.

yup, I must've copy/pasted the wrong thing.

> 
> Changed locally to
> 
> u32 tcr = dart->hw->tcr_enabled; if (levels == 4)        tcr |= dart->hw-
>> tcr_4level;
> 
> and then writel(tcr, ...) in the loop.
Great, I didn't even realize you could move that entire thing out of the 
loop.

> I've change prefix of all commits in this series to "iommu/apple-dart"
> and "iommu/io-pgtable-dart".

Feel free to add my Reviewed-by for this commit as well then.



Thanks,

Sven