Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space"

[Leizhen (ThunderTown)]

On 2021/1/20 23:02, Robin Murphy wrote:
> On 2021-01-19 01:59, Zhen Lei wrote:
>> This reverts commit 52f3fab0067d6fa9e99c1b7f63265dd48ca76046.
>>
>> This problem has been fixed by another patch. The original method had side
>> effects, it was not mapped to the user-specified resource size. The code
>> will become more complex when ECMDQ is supported later.
> 
> FWIW I don't think that's a significant issue either way - there could be any number of imp-def pages between SMMU page 0 and the ECMDQ control pages, so it will still be logical to map them as another separate thing anyway.

Yes, so now I'm thinking of preserving the SMMUv3 resources and eliminating the imp-def area. Then use another devm_ioremap() to cover the entire resource，assign it to smmu->base.
Otherwise, a base pointer needs to be defined for each separated register space，or call a function to convert each time.

> 
> Robin.
> 
>> Signed-off-by: Zhen Lei <thunder.leizhen@xxxxxxxxxx>
>> ---
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 32 ++++-------------------------
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 ---
>>   2 files changed, 4 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index 8ca7415d785d9bf..477f473842e5272 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -91,8 +91,9 @@ struct arm_smmu_option_prop {
>>   static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
>>                            struct arm_smmu_device *smmu)
>>   {
>> -    if (offset > SZ_64K)
>> -        return smmu->page1 + offset - SZ_64K;
>> +    if ((offset > SZ_64K) &&
>> +        (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY))
>> +        offset -= SZ_64K;
>>         return smmu->base + offset;
>>   }
>> @@ -3486,18 +3487,6 @@ static int arm_smmu_set_bus_ops(struct iommu_ops *ops)
>>       return err;
>>   }
>>   -static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
>> -                      resource_size_t size)
>> -{
>> -    struct resource res = {
>> -        .flags = IORESOURCE_MEM,
>> -        .start = start,
>> -        .end = start + size - 1,
>> -    };
>> -
>> -    return devm_ioremap_resource(dev, &res);
>> -}
>> -
>>   static int arm_smmu_device_probe(struct platform_device *pdev)
>>   {
>>       int irq, ret;
>> @@ -3533,23 +3522,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>       }
>>       ioaddr = res->start;
>>   -    /*
>> -     * Don't map the IMPLEMENTATION DEFINED regions, since they may contain
>> -     * the PMCG registers which are reserved by the PMU driver.
>> -     */
>> -    smmu->base = arm_smmu_ioremap(dev, ioaddr, ARM_SMMU_REG_SZ);
>> +    smmu->base = devm_ioremap_resource(dev, res);
>>       if (IS_ERR(smmu->base))
>>           return PTR_ERR(smmu->base);
>>   -    if (arm_smmu_resource_size(smmu) > SZ_64K) {
>> -        smmu->page1 = arm_smmu_ioremap(dev, ioaddr + SZ_64K,
>> -                           ARM_SMMU_REG_SZ);
>> -        if (IS_ERR(smmu->page1))
>> -            return PTR_ERR(smmu->page1);
>> -    } else {
>> -        smmu->page1 = smmu->base;
>> -    }
>> -
>>       /* Interrupt lines */
>>         irq = platform_get_irq_byname_optional(pdev, "combined");
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> index 96c2e9565e00282..0c3090c60840c22 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> @@ -152,8 +152,6 @@
>>   #define ARM_SMMU_PRIQ_IRQ_CFG1        0xd8
>>   #define ARM_SMMU_PRIQ_IRQ_CFG2        0xdc
>>   -#define ARM_SMMU_REG_SZ            0xe00
>> -
>>   /* Common MSI config fields */
>>   #define MSI_CFG0_ADDR_MASK        GENMASK_ULL(51, 2)
>>   #define MSI_CFG2_SH            GENMASK(5, 4)
>> @@ -584,7 +582,6 @@ struct arm_smmu_strtab_cfg {
>>   struct arm_smmu_device {
>>       struct device            *dev;
>>       void __iomem            *base;
>> -    void __iomem            *page1;
>>     #define ARM_SMMU_FEAT_2_LVL_STRTAB    (1 << 0)
>>   #define ARM_SMMU_FEAT_2_LVL_CDTAB    (1 << 1)
>>
> 
> .
>