| Message ID | 20160607033945.28687.70956.sendpatchset@little-apple (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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");
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
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
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
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.
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
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.
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.
--- 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");