Re: [PATCH v3 01/09] iommu/ipmmu-vmsa: Introduce features, break out alias

[Magnus Damm]

Hi Robin,

On Wed, Mar 8, 2017 at 8:53 PM, Robin Murphy <robin.murphy@xxxxxxx> wrote:
> Hi Magnus,
>
> On 08/03/17 11:01, Magnus Damm wrote:
>> From: Magnus Damm <damm+renesas@xxxxxxxxxxxxx>
>>
>> Introduce struct ipmmu_features to track various hardware
>> and software implementation changes inside the driver for
>> different kinds of IPMMU hardware. Add use_ns_alias_offset
>> as a first example of a feature to control if the secure
>> register bank offset should be used or not.
>>
>> Signed-off-by: Magnus Damm <damm+renesas@xxxxxxxxxxxxx>
>> ---
>>
>>  Changes since V2:
>>  - None
>>
>>  Changes since V1:
>>  - Moved patch to front of the series
>>
>>  drivers/iommu/ipmmu-vmsa.c |   35 ++++++++++++++++++++++++++++-------
>>  1 file changed, 28 insertions(+), 7 deletions(-)
>>
>> --- 0007/drivers/iommu/ipmmu-vmsa.c
>> +++ work/drivers/iommu/ipmmu-vmsa.c   2017-03-07 12:25:47.000000000 +0900
>> @@ -32,11 +32,15 @@
>>
>>  #define IPMMU_CTX_MAX 1
>>
>> +struct ipmmu_features {
>> +     bool use_ns_alias_offset;
>> +};
>> +
>>  struct ipmmu_vmsa_device {
>>       struct device *dev;
>>       void __iomem *base;
>>       struct list_head list;
>> -
>> +     const struct ipmmu_features *features;
>>       unsigned int num_utlbs;
>>       spinlock_t lock;                        /* Protects ctx and domains[] */
>>       DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
>> @@ -999,13 +1003,33 @@ static void ipmmu_device_reset(struct ip
>>               ipmmu_write(mmu, i * IM_CTX_SIZE + IMCTR, 0);
>>  }
>>
>> +static const struct ipmmu_features ipmmu_features_default = {
>> +     .use_ns_alias_offset = true,
>> +};
>> +
>> +static const struct of_device_id ipmmu_of_ids[] = {
>> +     {
>> +             .compatible = "renesas,ipmmu-vmsa",
>> +             .data = &ipmmu_features_default,
>> +     }, {
>> +             /* Terminator */
>> +     },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, ipmmu_of_ids);
>> +
>>  static int ipmmu_probe(struct platform_device *pdev)
>>  {
>>       struct ipmmu_vmsa_device *mmu;
>> +     const struct of_device_id *match;
>>       struct resource *res;
>>       int irq;
>>       int ret;
>>
>> +     match = of_match_node(ipmmu_of_ids, pdev->dev.of_node);
>
> of_device_get_match_data() makes this a lot easier.
>
>> +     if (!match)
>> +             return -EINVAL;
>
> Also, if the driver is DT-only per the other series, note that this
> cannot happen anyway, since of_driver_match_device() would have to have
> found a match for your probe function to be called in the first place.

Yeah, you are right. As you know, in the IPMMU driver (with the
r8a7795 V3 series applied) the init handling is a bit special with
ARM32 and ARM64 being treated differently. I would like to clean it up
and share a common implementation.

Until that happens, how do you think we should handle the (!match)
case? BUG_ON()?

Cheers,

/ magnus