diff mbox

[3/3] iommu/ipmmu-vmsa: Hook up r8a7796 DT matching code

Message ID 20160607033945.28687.70956.sendpatchset@little-apple (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Magnus Damm June 7, 2016, 3:39 a.m. UTC
From: Magnus Damm <damm+renesas@opensource.se>

Support the r8a7796 IPMMU by sharing feature flags between
r8a7795 and r8a7796. Also update IOMMU_OF_DECLARE to hook
up the updated compat string.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 drivers/iommu/ipmmu-vmsa.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart June 8, 2016, 12:18 a.m. UTC | #1
Hi Magnus,

Thank you for the patch.

On Tuesday 07 Jun 2016 12:39:45 Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Support the r8a7796 IPMMU by sharing feature flags between
> r8a7795 and r8a7796. Also update IOMMU_OF_DECLARE to hook
> up the updated compat string.
> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  drivers/iommu/ipmmu-vmsa.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> --- 0031/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2016-06-06 11:19:40.210607110 +0900
> @@ -1074,7 +1074,7 @@ static const struct ipmmu_features ipmmu
>  	.twobit_imttbcr_sl0 = false,
>  };
> 
> -static const struct ipmmu_features ipmmu_features_r8a7795 = {
> +static const struct ipmmu_features ipmmu_features_rcar_gen3 = {
>  	.use_ns_alias_offset = false,
>  	.has_cache_leaf_nodes = true,
>  	.has_eight_ctx = true,
> @@ -1088,7 +1088,10 @@ static const struct of_device_id ipmmu_o
>  		.data = &ipmmu_features_default,
>  	}, {
>  		.compatible = "renesas,ipmmu-r8a7795",
> -		.data = &ipmmu_features_r8a7795,
> +		.data = &ipmmu_features_rcar_gen3,
> +	}, {
> +		.compatible = "renesas,ipmmu-r8a7796",
> +		.data = &ipmmu_features_rcar_gen3,
>  	}, {
>  		/* Terminator */
>  	},
> @@ -1268,6 +1271,8 @@ IOMMU_OF_DECLARE(ipmmu_vmsa_iommu_of, "r
>  		 ipmmu_vmsa_iommu_of_setup);
>  IOMMU_OF_DECLARE(ipmmu_r8a7795_iommu_of, "renesas,ipmmu-r8a7795",
>  		 ipmmu_vmsa_iommu_of_setup);
> +IOMMU_OF_DECLARE(ipmmu_r8a7796_iommu_of, "renesas,ipmmu-r8a7796",
> +		 ipmmu_vmsa_iommu_of_setup);

How about a Gen3 generic compatible string in addition to the SoC-specific 
ones ?

>  #endif
> 
>  MODULE_DESCRIPTION("IOMMU API for Renesas VMSA-compatible IPMMU");
Geert Uytterhoeven June 8, 2016, 7:04 a.m. UTC | #2
Hi Laurent,

On Wed, Jun 8, 2016 at 2:18 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> --- 0031/drivers/iommu/ipmmu-vmsa.c
>> +++ work/drivers/iommu/ipmmu-vmsa.c   2016-06-06 11:19:40.210607110 +0900
>> @@ -1074,7 +1074,7 @@ static const struct ipmmu_features ipmmu
>>       .twobit_imttbcr_sl0 = false,
>>  };
>>
>> -static const struct ipmmu_features ipmmu_features_r8a7795 = {
>> +static const struct ipmmu_features ipmmu_features_rcar_gen3 = {
>>       .use_ns_alias_offset = false,
>>       .has_cache_leaf_nodes = true,
>>       .has_eight_ctx = true,
>> @@ -1088,7 +1088,10 @@ static const struct of_device_id ipmmu_o
>>               .data = &ipmmu_features_default,
>>       }, {
>>               .compatible = "renesas,ipmmu-r8a7795",
>> -             .data = &ipmmu_features_r8a7795,
>> +             .data = &ipmmu_features_rcar_gen3,
>> +     }, {
>> +             .compatible = "renesas,ipmmu-r8a7796",
>> +             .data = &ipmmu_features_rcar_gen3,
>>       }, {
>>               /* Terminator */
>>       },
>> @@ -1268,6 +1271,8 @@ IOMMU_OF_DECLARE(ipmmu_vmsa_iommu_of, "r
>>                ipmmu_vmsa_iommu_of_setup);
>>  IOMMU_OF_DECLARE(ipmmu_r8a7795_iommu_of, "renesas,ipmmu-r8a7795",
>>                ipmmu_vmsa_iommu_of_setup);
>> +IOMMU_OF_DECLARE(ipmmu_r8a7796_iommu_of, "renesas,ipmmu-r8a7796",
>> +              ipmmu_vmsa_iommu_of_setup);
>
> How about a Gen3 generic compatible string in addition to the SoC-specific
> ones ?

Do we want to specify the number of utlbs here?
Does it differ between r8a7795, r8a7796, and future members?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Magnus Damm June 8, 2016, 8:05 a.m. UTC | #3
Hi Geert,

On Wed, Jun 8, 2016 at 4:04 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Laurent,
>
> On Wed, Jun 8, 2016 at 2:18 AM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>> --- 0031/drivers/iommu/ipmmu-vmsa.c
>>> +++ work/drivers/iommu/ipmmu-vmsa.c   2016-06-06 11:19:40.210607110 +0900
>>> @@ -1074,7 +1074,7 @@ static const struct ipmmu_features ipmmu
>>>       .twobit_imttbcr_sl0 = false,
>>>  };
>>>
>>> -static const struct ipmmu_features ipmmu_features_r8a7795 = {
>>> +static const struct ipmmu_features ipmmu_features_rcar_gen3 = {
>>>       .use_ns_alias_offset = false,
>>>       .has_cache_leaf_nodes = true,
>>>       .has_eight_ctx = true,
>>> @@ -1088,7 +1088,10 @@ static const struct of_device_id ipmmu_o
>>>               .data = &ipmmu_features_default,
>>>       }, {
>>>               .compatible = "renesas,ipmmu-r8a7795",
>>> -             .data = &ipmmu_features_r8a7795,
>>> +             .data = &ipmmu_features_rcar_gen3,
>>> +     }, {
>>> +             .compatible = "renesas,ipmmu-r8a7796",
>>> +             .data = &ipmmu_features_rcar_gen3,
>>>       }, {
>>>               /* Terminator */
>>>       },
>>> @@ -1268,6 +1271,8 @@ IOMMU_OF_DECLARE(ipmmu_vmsa_iommu_of, "r
>>>                ipmmu_vmsa_iommu_of_setup);
>>>  IOMMU_OF_DECLARE(ipmmu_r8a7795_iommu_of, "renesas,ipmmu-r8a7795",
>>>                ipmmu_vmsa_iommu_of_setup);
>>> +IOMMU_OF_DECLARE(ipmmu_r8a7796_iommu_of, "renesas,ipmmu-r8a7796",
>>> +              ipmmu_vmsa_iommu_of_setup);
>>
>> How about a Gen3 generic compatible string in addition to the SoC-specific
>> ones ?
>
> Do we want to specify the number of utlbs here?
> Does it differ between r8a7795, r8a7796, and future members?

The utlb number is already a property of the SoC part number. So I
don't see why we want to encode this as a separate DT property instead
of going with an in-driver feature flag.

Thanks,

/ magnus
Magnus Damm June 8, 2016, 8:07 a.m. UTC | #4
Hi Laurent,

On Wed, Jun 8, 2016 at 9:18 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> Thank you for the patch.
>
> On Tuesday 07 Jun 2016 12:39:45 Magnus Damm wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> Support the r8a7796 IPMMU by sharing feature flags between
>> r8a7795 and r8a7796. Also update IOMMU_OF_DECLARE to hook
>> up the updated compat string.
>>
>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> ---
>>
>>  drivers/iommu/ipmmu-vmsa.c |    9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> --- 0031/drivers/iommu/ipmmu-vmsa.c
>> +++ work/drivers/iommu/ipmmu-vmsa.c   2016-06-06 11:19:40.210607110 +0900
>> @@ -1074,7 +1074,7 @@ static const struct ipmmu_features ipmmu
>>       .twobit_imttbcr_sl0 = false,
>>  };
>>
>> -static const struct ipmmu_features ipmmu_features_r8a7795 = {
>> +static const struct ipmmu_features ipmmu_features_rcar_gen3 = {
>>       .use_ns_alias_offset = false,
>>       .has_cache_leaf_nodes = true,
>>       .has_eight_ctx = true,
>> @@ -1088,7 +1088,10 @@ static const struct of_device_id ipmmu_o
>>               .data = &ipmmu_features_default,
>>       }, {
>>               .compatible = "renesas,ipmmu-r8a7795",
>> -             .data = &ipmmu_features_r8a7795,
>> +             .data = &ipmmu_features_rcar_gen3,
>> +     }, {
>> +             .compatible = "renesas,ipmmu-r8a7796",
>> +             .data = &ipmmu_features_rcar_gen3,
>>       }, {
>>               /* Terminator */
>>       },
>> @@ -1268,6 +1271,8 @@ IOMMU_OF_DECLARE(ipmmu_vmsa_iommu_of, "r
>>                ipmmu_vmsa_iommu_of_setup);
>>  IOMMU_OF_DECLARE(ipmmu_r8a7795_iommu_of, "renesas,ipmmu-r8a7795",
>>                ipmmu_vmsa_iommu_of_setup);
>> +IOMMU_OF_DECLARE(ipmmu_r8a7796_iommu_of, "renesas,ipmmu-r8a7796",
>> +              ipmmu_vmsa_iommu_of_setup);
>
> How about a Gen3 generic compatible string in addition to the SoC-specific
> ones ?

As you probably know, neither R-Car Gen2 nor R-Car Gen3 families are
finished evolving. Because of that I find speculating with DT compat
strings just potentially confusing with no apparent upside. Just going
with what we know (ie the part number) is more than good enough IMO.

Thanks,

/ magnus
Laurent Pinchart June 8, 2016, 8:48 a.m. UTC | #5
Hi Geert,

On Wednesday 08 Jun 2016 09:04:17 Geert Uytterhoeven wrote:
> On Wed, Jun 8, 2016 at 2:18 AM, Laurent Pinchart wrote:
> >> --- 0031/drivers/iommu/ipmmu-vmsa.c
> >> +++ work/drivers/iommu/ipmmu-vmsa.c   2016-06-06 11:19:40.210607110 +0900
> >> @@ -1074,7 +1074,7 @@ static const struct ipmmu_features ipmmu
> >>       .twobit_imttbcr_sl0 = false,
 >>  };
> >> 
> >> -static const struct ipmmu_features ipmmu_features_r8a7795 = {
> >> +static const struct ipmmu_features ipmmu_features_rcar_gen3 = {
> >>       .use_ns_alias_offset = false,
> >>       .has_cache_leaf_nodes = true,
> >>       .has_eight_ctx = true,
> >> @@ -1088,7 +1088,10 @@ static const struct of_device_id ipmmu_o
> >>               .data = &ipmmu_features_default,
> >>       }, {
> >>               .compatible = "renesas,ipmmu-r8a7795",
> >> -             .data = &ipmmu_features_r8a7795,
> >> +             .data = &ipmmu_features_rcar_gen3,
> >> +     }, {
> >> +             .compatible = "renesas,ipmmu-r8a7796",
> >> +             .data = &ipmmu_features_rcar_gen3,
> >>       }, {
> >>               /* Terminator */
> >>       },
> >> @@ -1268,6 +1271,8 @@ IOMMU_OF_DECLARE(ipmmu_vmsa_iommu_of, "r
> >>                ipmmu_vmsa_iommu_of_setup);
> >>  IOMMU_OF_DECLARE(ipmmu_r8a7795_iommu_of, "renesas,ipmmu-r8a7795",
> >>                ipmmu_vmsa_iommu_of_setup);
> >> +IOMMU_OF_DECLARE(ipmmu_r8a7796_iommu_of, "renesas,ipmmu-r8a7796",
> >> +              ipmmu_vmsa_iommu_of_setup);
> > 
> > How about a Gen3 generic compatible string in addition to the SoC-specific
> > ones ?
> 
> Do we want to specify the number of utlbs here?
> Does it differ between r8a7795, r8a7796, and future members?

It differs between IPMMU instances on a given SoC, so if we want to specify it 
it should be a DT property.
Magnus Damm June 8, 2016, 9:12 a.m. UTC | #6
Hi Laurent,

On Wed, Jun 8, 2016 at 5:48 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Geert,
>
> On Wednesday 08 Jun 2016 09:04:17 Geert Uytterhoeven wrote:
>> On Wed, Jun 8, 2016 at 2:18 AM, Laurent Pinchart wrote:
>> >> --- 0031/drivers/iommu/ipmmu-vmsa.c
>> >> +++ work/drivers/iommu/ipmmu-vmsa.c   2016-06-06 11:19:40.210607110 +0900
>> >> @@ -1074,7 +1074,7 @@ static const struct ipmmu_features ipmmu
>> >>       .twobit_imttbcr_sl0 = false,
>  >>  };
>> >>
>> >> -static const struct ipmmu_features ipmmu_features_r8a7795 = {
>> >> +static const struct ipmmu_features ipmmu_features_rcar_gen3 = {
>> >>       .use_ns_alias_offset = false,
>> >>       .has_cache_leaf_nodes = true,
>> >>       .has_eight_ctx = true,
>> >> @@ -1088,7 +1088,10 @@ static const struct of_device_id ipmmu_o
>> >>               .data = &ipmmu_features_default,
>> >>       }, {
>> >>               .compatible = "renesas,ipmmu-r8a7795",
>> >> -             .data = &ipmmu_features_r8a7795,
>> >> +             .data = &ipmmu_features_rcar_gen3,
>> >> +     }, {
>> >> +             .compatible = "renesas,ipmmu-r8a7796",
>> >> +             .data = &ipmmu_features_rcar_gen3,
>> >>       }, {
>> >>               /* Terminator */
>> >>       },
>> >> @@ -1268,6 +1271,8 @@ IOMMU_OF_DECLARE(ipmmu_vmsa_iommu_of, "r
>> >>                ipmmu_vmsa_iommu_of_setup);
>> >>  IOMMU_OF_DECLARE(ipmmu_r8a7795_iommu_of, "renesas,ipmmu-r8a7795",
>> >>                ipmmu_vmsa_iommu_of_setup);
>> >> +IOMMU_OF_DECLARE(ipmmu_r8a7796_iommu_of, "renesas,ipmmu-r8a7796",
>> >> +              ipmmu_vmsa_iommu_of_setup);
>> >
>> > How about a Gen3 generic compatible string in addition to the SoC-specific
>> > ones ?
>>
>> Do we want to specify the number of utlbs here?
>> Does it differ between r8a7795, r8a7796, and future members?
>
> It differs between IPMMU instances on a given SoC, so if we want to specify it
> it should be a DT property.

Can you please point out which documentation that says it varies with
IPMMU instance?

Based on IMUCTRn register description "H3-ES1" has 0-31 range while
"Others" have 0-47.

Thanks,

/ magnus
Laurent Pinchart Aug. 9, 2016, 1:17 p.m. UTC | #7
Hi Magnus,

On Wednesday 08 Jun 2016 18:12:31 Magnus Damm wrote:
> On Wed, Jun 8, 2016 at 5:48 PM, Laurent Pinchart wrote:
> > On Wednesday 08 Jun 2016 09:04:17 Geert Uytterhoeven wrote:
> >> On Wed, Jun 8, 2016 at 2:18 AM, Laurent Pinchart wrote:
> >>>> --- 0031/drivers/iommu/ipmmu-vmsa.c
> >>>> +++ work/drivers/iommu/ipmmu-vmsa.c   2016-06-06 11:19:40.210607110

[snip]

> >>>> @@ -1268,6 +1271,8 @@ IOMMU_OF_DECLARE(ipmmu_vmsa_iommu_of, "r
> >>>>                ipmmu_vmsa_iommu_of_setup);
> >>>>  IOMMU_OF_DECLARE(ipmmu_r8a7795_iommu_of, "renesas,ipmmu-r8a7795",
> >>>>                ipmmu_vmsa_iommu_of_setup);
> >>>> +IOMMU_OF_DECLARE(ipmmu_r8a7796_iommu_of, "renesas,ipmmu-r8a7796",
> >>>> +              ipmmu_vmsa_iommu_of_setup);
> >>> 
> >>> How about a Gen3 generic compatible string in addition to the
> >>> SoC-specific ones ?
> >> 
> >> Do we want to specify the number of utlbs here?
> >> Does it differ between r8a7795, r8a7796, and future members?
> > 
> > It differs between IPMMU instances on a given SoC, so if we want to
> > specify it it should be a DT property.
> 
> Can you please point out which documentation that says it varies with
> IPMMU instance?
> 
> Based on IMUCTRn register description "H3-ES1" has 0-31 range while
> "Others" have 0-47.

The maximum number of uTLBs is indeed the same according to that part of the 
documentation, but not all uTLBs are available in all IPMMU instances. We even 
have holes in the uTLB ranges, maybe a mask would be more appropriate.
Laurent Pinchart Aug. 9, 2016, 1:19 p.m. UTC | #8
On Tuesday 09 Aug 2016 16:17:57 Laurent Pinchart wrote:
> On Wednesday 08 Jun 2016 18:12:31 Magnus Damm wrote:
> > On Wed, Jun 8, 2016 at 5:48 PM, Laurent Pinchart wrote:
> >> On Wednesday 08 Jun 2016 09:04:17 Geert Uytterhoeven wrote:
> >>> On Wed, Jun 8, 2016 at 2:18 AM, Laurent Pinchart wrote:
> >>>>> --- 0031/drivers/iommu/ipmmu-vmsa.c
> >>>>> +++ work/drivers/iommu/ipmmu-vmsa.c   2016-06-06 11:19:40.210607110
> 
> [snip]
> 
> >>>>> @@ -1268,6 +1271,8 @@ IOMMU_OF_DECLARE(ipmmu_vmsa_iommu_of, "r
> >>>>>                ipmmu_vmsa_iommu_of_setup);
> >>>>>  IOMMU_OF_DECLARE(ipmmu_r8a7795_iommu_of, "renesas,ipmmu-r8a7795",
> >>>>>                ipmmu_vmsa_iommu_of_setup);
> >>>>> +IOMMU_OF_DECLARE(ipmmu_r8a7796_iommu_of, "renesas,ipmmu-r8a7796",
> >>>>> +              ipmmu_vmsa_iommu_of_setup);
> >>>> 
> >>>> How about a Gen3 generic compatible string in addition to the
> >>>> SoC-specific ones ?
> >>> 
> >>> Do we want to specify the number of utlbs here?
> >>> Does it differ between r8a7795, r8a7796, and future members?
> >> 
> >> It differs between IPMMU instances on a given SoC, so if we want to
> >> specify it it should be a DT property.
> > 
> > Can you please point out which documentation that says it varies with
> > IPMMU instance?
> > 
> > Based on IMUCTRn register description "H3-ES1" has 0-31 range while
> > "Others" have 0-47.
> 
> The maximum number of uTLBs is indeed the same according to that part of the
> documentation, but not all uTLBs are available in all IPMMU instances. We
> even have holes in the uTLB ranges, maybe a mask would be more appropriate.

And the other option is of course to ignore that and accept any uTLB number, 
in which case we will rely on the IOMMU bus master node providing correct 
information.
diff mbox

Patch

--- 0031/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2016-06-06 11:19:40.210607110 +0900
@@ -1074,7 +1074,7 @@  static const struct ipmmu_features ipmmu
 	.twobit_imttbcr_sl0 = false,
 };
 
-static const struct ipmmu_features ipmmu_features_r8a7795 = {
+static const struct ipmmu_features ipmmu_features_rcar_gen3 = {
 	.use_ns_alias_offset = false,
 	.has_cache_leaf_nodes = true,
 	.has_eight_ctx = true,
@@ -1088,7 +1088,10 @@  static const struct of_device_id ipmmu_o
 		.data = &ipmmu_features_default,
 	}, {
 		.compatible = "renesas,ipmmu-r8a7795",
-		.data = &ipmmu_features_r8a7795,
+		.data = &ipmmu_features_rcar_gen3,
+	}, {
+		.compatible = "renesas,ipmmu-r8a7796",
+		.data = &ipmmu_features_rcar_gen3,
 	}, {
 		/* Terminator */
 	},
@@ -1268,6 +1271,8 @@  IOMMU_OF_DECLARE(ipmmu_vmsa_iommu_of, "r
 		 ipmmu_vmsa_iommu_of_setup);
 IOMMU_OF_DECLARE(ipmmu_r8a7795_iommu_of, "renesas,ipmmu-r8a7795",
 		 ipmmu_vmsa_iommu_of_setup);
+IOMMU_OF_DECLARE(ipmmu_r8a7796_iommu_of, "renesas,ipmmu-r8a7796",
+		 ipmmu_vmsa_iommu_of_setup);
 #endif
 
 MODULE_DESCRIPTION("IOMMU API for Renesas VMSA-compatible IPMMU");