From: Marian-Cristian Rotariu <[email protected]>
Add support for RZ/G2H (R8A774E1) SoC IPMMUs.
Signed-off-by: Marian-Cristian Rotariu <[email protected]>
Signed-off-by: Lad Prabhakar <[email protected]>
---
drivers/iommu/ipmmu-vmsa.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index b90cd9ff96f6..cbce88ac5a71 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -751,6 +751,7 @@ static const struct soc_device_attribute soc_rcar_gen3[] = {
static const struct soc_device_attribute soc_rcar_gen3_whitelist[] = {
{ .soc_id = "r8a774b1", },
{ .soc_id = "r8a774c0", },
+ { .soc_id = "r8a774e1", },
{ .soc_id = "r8a7795", .revision = "ES3.*" },
{ .soc_id = "r8a77961", },
{ .soc_id = "r8a77965", },
@@ -963,6 +964,9 @@ static const struct of_device_id ipmmu_of_ids[] = {
}, {
.compatible = "renesas,ipmmu-r8a774c0",
.data = &ipmmu_features_rcar_gen3,
+ }, {
+ .compatible = "renesas,ipmmu-r8a774e1",
+ .data = &ipmmu_features_rcar_gen3,
}, {
.compatible = "renesas,ipmmu-r8a7795",
.data = &ipmmu_features_rcar_gen3,
--
2.17.1
Hi Prabhakar,
On Mon, Jul 13, 2020 at 11:35 PM Lad Prabhakar
<[email protected]> wrote:
> From: Marian-Cristian Rotariu <[email protected]>
>
> Add support for RZ/G2H (R8A774E1) SoC IPMMUs.
>
> Signed-off-by: Marian-Cristian Rotariu <[email protected]>
> Signed-off-by: Lad Prabhakar <[email protected]>
Thanks for your patch!
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -751,6 +751,7 @@ static const struct soc_device_attribute soc_rcar_gen3[] = {
> static const struct soc_device_attribute soc_rcar_gen3_whitelist[] = {
> { .soc_id = "r8a774b1", },
> { .soc_id = "r8a774c0", },
> + { .soc_id = "r8a774e1", },
Adding an entry to soc_rcar_gen3_whitelist[] doesn't do anything, unless
you also add the same entry to soc_rcar_gen3[].
> { .soc_id = "r8a7795", .revision = "ES3.*" },
> { .soc_id = "r8a77961", },
> { .soc_id = "r8a77965", },
> @@ -963,6 +964,9 @@ static const struct of_device_id ipmmu_of_ids[] = {
> }, {
> .compatible = "renesas,ipmmu-r8a774c0",
> .data = &ipmmu_features_rcar_gen3,
> + }, {
> + .compatible = "renesas,ipmmu-r8a774e1",
> + .data = &ipmmu_features_rcar_gen3,
> }, {
> .compatible = "renesas,ipmmu-r8a7795",
> .data = &ipmmu_features_rcar_gen3,
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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,
Thank you for the review.
On Tue, Jul 14, 2020 at 9:09 AM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Mon, Jul 13, 2020 at 11:35 PM Lad Prabhakar
> <[email protected]> wrote:
> > From: Marian-Cristian Rotariu <[email protected]>
> >
> > Add support for RZ/G2H (R8A774E1) SoC IPMMUs.
> >
> > Signed-off-by: Marian-Cristian Rotariu <[email protected]>
> > Signed-off-by: Lad Prabhakar <[email protected]>
>
> Thanks for your patch!
>
> > --- a/drivers/iommu/ipmmu-vmsa.c
> > +++ b/drivers/iommu/ipmmu-vmsa.c
> > @@ -751,6 +751,7 @@ static const struct soc_device_attribute soc_rcar_gen3[] = {
> > static const struct soc_device_attribute soc_rcar_gen3_whitelist[] = {
> > { .soc_id = "r8a774b1", },
> > { .soc_id = "r8a774c0", },
> > + { .soc_id = "r8a774e1", },
>
> Adding an entry to soc_rcar_gen3_whitelist[] doesn't do anything, unless
> you also add the same entry to soc_rcar_gen3[].
>
I think the comment "For R-Car Gen3 use a white list to opt-in slave
devices." is misleading. Booting through the kernel I do see iommu
groups (attached is the logs). Also the recent patch to add
"r8a77961" just adds to soc_rcar_gen3_whitelist.
Cheers,
--Prabhakar
> > { .soc_id = "r8a7795", .revision = "ES3.*" },
> > { .soc_id = "r8a77961", },
> > { .soc_id = "r8a77965", },
> > @@ -963,6 +964,9 @@ static const struct of_device_id ipmmu_of_ids[] = {
> > }, {
> > .compatible = "renesas,ipmmu-r8a774c0",
> > .data = &ipmmu_features_rcar_gen3,
> > + }, {
> > + .compatible = "renesas,ipmmu-r8a774e1",
> > + .data = &ipmmu_features_rcar_gen3,
> > }, {
> > .compatible = "renesas,ipmmu-r8a7795",
> > .data = &ipmmu_features_rcar_gen3,
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> 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 Prabhakar,
On Tue, Jul 14, 2020 at 10:30 AM Lad, Prabhakar
<[email protected]> wrote:
> On Tue, Jul 14, 2020 at 9:09 AM Geert Uytterhoeven <[email protected]> wrote:
> > On Mon, Jul 13, 2020 at 11:35 PM Lad Prabhakar
> > <[email protected]> wrote:
> > > From: Marian-Cristian Rotariu <[email protected]>
> > >
> > > Add support for RZ/G2H (R8A774E1) SoC IPMMUs.
> > >
> > > Signed-off-by: Marian-Cristian Rotariu <[email protected]>
> > > Signed-off-by: Lad Prabhakar <[email protected]>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/iommu/ipmmu-vmsa.c
> > > +++ b/drivers/iommu/ipmmu-vmsa.c
> > > @@ -751,6 +751,7 @@ static const struct soc_device_attribute soc_rcar_gen3[] = {
> > > static const struct soc_device_attribute soc_rcar_gen3_whitelist[] = {
> > > { .soc_id = "r8a774b1", },
> > > { .soc_id = "r8a774c0", },
> > > + { .soc_id = "r8a774e1", },
> >
> > Adding an entry to soc_rcar_gen3_whitelist[] doesn't do anything, unless
> > you also add the same entry to soc_rcar_gen3[].
> >
> I think the comment "For R-Car Gen3 use a white list to opt-in slave
> devices." is misleading. Booting through the kernel I do see iommu
> groups (attached is the logs).
Indeed. Without an entry in soc_rcar_gen3[], the IPMMU is enabled
unconditionally, and soc_rcar_gen3_whitelist[] is ignored.
That's why you want an entry in both, unless you have an R-Car Gen3
SoC where the IPMMU works correctly with all slave devices present.
Perhaps soc_rcar_gen3[] should be renamed to soc_rcar_gen3_greylist[]
(or soc_rcar_gen3_maybelist[]) to make this clear?
> Also the recent patch to add
> "r8a77961" just adds to soc_rcar_gen3_whitelist.
Oops, commit 17fe16181639801b ("iommu/renesas: Add support for r8a77961")
did it wrong, too.
> > > { .soc_id = "r8a7795", .revision = "ES3.*" },
> > > { .soc_id = "r8a77961", },
> > > { .soc_id = "r8a77965", },
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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-san,
> From: Geert Uytterhoeven, Sent: Tuesday, July 14, 2020 5:42 PM
>
> Hi Prabhakar,
>
> On Tue, Jul 14, 2020 at 10:30 AM Lad, Prabhakar
> <[email protected]> wrote:
> > On Tue, Jul 14, 2020 at 9:09 AM Geert Uytterhoeven <[email protected]> wrote:
> > > On Mon, Jul 13, 2020 at 11:35 PM Lad Prabhakar
> > > <[email protected]> wrote:
> > > > From: Marian-Cristian Rotariu <[email protected]>
> > > >
> > > > Add support for RZ/G2H (R8A774E1) SoC IPMMUs.
> > > >
> > > > Signed-off-by: Marian-Cristian Rotariu <[email protected]>
> > > > Signed-off-by: Lad Prabhakar <[email protected]>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/iommu/ipmmu-vmsa.c
> > > > +++ b/drivers/iommu/ipmmu-vmsa.c
> > > > @@ -751,6 +751,7 @@ static const struct soc_device_attribute soc_rcar_gen3[] = {
> > > > static const struct soc_device_attribute soc_rcar_gen3_whitelist[] = {
> > > > { .soc_id = "r8a774b1", },
> > > > { .soc_id = "r8a774c0", },
> > > > + { .soc_id = "r8a774e1", },
> > >
> > > Adding an entry to soc_rcar_gen3_whitelist[] doesn't do anything, unless
> > > you also add the same entry to soc_rcar_gen3[].
> > >
> > I think the comment "For R-Car Gen3 use a white list to opt-in slave
> > devices." is misleading. Booting through the kernel I do see iommu
> > groups (attached is the logs).
>
> Indeed. Without an entry in soc_rcar_gen3[], the IPMMU is enabled
> unconditionally, and soc_rcar_gen3_whitelist[] is ignored.
> That's why you want an entry in both, unless you have an R-Car Gen3
> SoC where the IPMMU works correctly with all slave devices present.
> Perhaps soc_rcar_gen3[] should be renamed to soc_rcar_gen3_greylist[]
> (or soc_rcar_gen3_maybelist[]) to make this clear?
I think so (we should rename it).
> > Also the recent patch to add
> > "r8a77961" just adds to soc_rcar_gen3_whitelist.
>
> Oops, commit 17fe16181639801b ("iommu/renesas: Add support for r8a77961")
> did it wrong, too.
Thank you for the point it out. We should add r8a77961 to the soc_rcar_gen3[].
However, I don't know why I could not realize this issue...
So, I investigated this a little and then, IIUC, glob_match() which
soc_device_match() uses seems to return true, if *pat = "r8a7796" and *str = "r8a77961".
Best regards,
Yoshihiro Shimoda
Hi Shimoda-san,
On Tue, Jul 14, 2020 at 1:42 PM Yoshihiro Shimoda
<[email protected]> wrote:
> > From: Geert Uytterhoeven, Sent: Tuesday, July 14, 2020 5:42 PM
> > On Tue, Jul 14, 2020 at 10:30 AM Lad, Prabhakar
> > <[email protected]> wrote:
> > > On Tue, Jul 14, 2020 at 9:09 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > On Mon, Jul 13, 2020 at 11:35 PM Lad Prabhakar
> > > Also the recent patch to add
> > > "r8a77961" just adds to soc_rcar_gen3_whitelist.
> >
> > Oops, commit 17fe16181639801b ("iommu/renesas: Add support for r8a77961")
> > did it wrong, too.
>
> Thank you for the point it out. We should add r8a77961 to the soc_rcar_gen3[].
> However, I don't know why I could not realize this issue...
> So, I investigated this a little and then, IIUC, glob_match() which
> soc_device_match() uses seems to return true, if *pat = "r8a7796" and *str = "r8a77961".
Are you sure about this?
I enabled CONFIG_GLOB_SELFTEST, and globtest succeeded.
It does test glob_match("a", "aa"), which is a similar test.
To be 100% sure, I added:
--- a/lib/globtest.c
+++ b/lib/globtest.c
@@ -59,6 +59,7 @@ static char const glob_tests[] __initconst =
"1" "a\0" "a\0"
"0" "a\0" "b\0"
"0" "a\0" "aa\0"
+ "0" "r8a7796\0" "r8a77961\0"
"0" "a\0" "\0"
"1" "\0" "\0"
"0" "\0" "a\0"
and it still succeeded.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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-san,
> From: Geert Uytterhoeven, Sent: Tuesday, July 14, 2020 9:40 PM
>
> Hi Shimoda-san,
>
> On Tue, Jul 14, 2020 at 1:42 PM Yoshihiro Shimoda
> <[email protected]> wrote:
> > > From: Geert Uytterhoeven, Sent: Tuesday, July 14, 2020 5:42 PM
> > > On Tue, Jul 14, 2020 at 10:30 AM Lad, Prabhakar
> > > <[email protected]> wrote:
> > > > On Tue, Jul 14, 2020 at 9:09 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > > On Mon, Jul 13, 2020 at 11:35 PM Lad Prabhakar
> > > > Also the recent patch to add
> > > > "r8a77961" just adds to soc_rcar_gen3_whitelist.
> > >
> > > Oops, commit 17fe16181639801b ("iommu/renesas: Add support for r8a77961")
> > > did it wrong, too.
> >
> > Thank you for the point it out. We should add r8a77961 to the soc_rcar_gen3[].
> > However, I don't know why I could not realize this issue...
> > So, I investigated this a little and then, IIUC, glob_match() which
> > soc_device_match() uses seems to return true, if *pat = "r8a7796" and *str = "r8a77961".
>
> Are you sure about this?
I'm very sorry. I completely misunderstood the glob_match() behavior.
And, now I understood why the current code can use IPMMU on r8a77961...
# Since the first soc_device_match() will return false, ipmmu_slave_whitelist()
# will return true and then the ipmmu_of_xlate() will be succeeded.
> I enabled CONFIG_GLOB_SELFTEST, and globtest succeeded.
> It does test glob_match("a", "aa"), which is a similar test.
>
> To be 100% sure, I added:
>
> --- a/lib/globtest.c
> +++ b/lib/globtest.c
> @@ -59,6 +59,7 @@ static char const glob_tests[] __initconst =
> "1" "a\0" "a\0"
> "0" "a\0" "b\0"
> "0" "a\0" "aa\0"
> + "0" "r8a7796\0" "r8a77961\0"
> "0" "a\0" "\0"
> "1" "\0" "\0"
> "0" "\0" "a\0"
>
> and it still succeeded.
I'm very sorry to waste your time about this...
Best regards,
Yoshihiro Shimoda