RE: [PATCH 2/9] iommu/ipmmu-vmsa: Hook up R8A774E1 DT matching code
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
> wrote:
> > > From: Geert Uytterhoeven, Sent: Tuesday, July 14, 2020 5:42 PM
> > > On Tue, Jul 14, 2020 at 10:30 AM Lad, Prabhakar
> > > wrote:
> > > > On Tue, Jul 14, 2020 at 9:09 AM Geert Uytterhoeven
> > > > 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/9] iommu/ipmmu-vmsa: Hook up R8A774E1 DT matching code
Hi Geert,
Thank you for the review.
On Tue, Jul 14, 2020 at 9:09 AM Geert Uytterhoeven wrote:
>
> Hi Prabhakar,
>
> On Mon, Jul 13, 2020 at 11:35 PM Lad Prabhakar
> wrote:
> > From: Marian-Cristian Rotariu
> >
> > Add support for RZ/G2H (R8A774E1) SoC IPMMUs.
> >
> > Signed-off-by: Marian-Cristian Rotariu
> >
> > Signed-off-by: Lad Prabhakar
>
> 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 --
> ge...@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
[0.000149] NOTICE: BL2: RZ G2H Initial Program Loader(CA57)
[0.004417] NOTICE: BL2: Initial Program Loader(Rev.2.0.6)
[0.009949] NOTICE: BL2: PRR is RZG G2H Ver.3.0
[0.014530] NOTICE: BL2: Board is HiHope RZ/G2H Rev.4.0
[0.019796] NOTICE: BL2: Boot device is QSPI Flash(40MHz)
[0.025236] NOTICE: BL2: LCM state is CM
[0.029203] NOTICE: BL2: CH0: 0x4 - 0x47fff, 2 GiB
[0.035071] NOTICE: BL2: CH1: 0x5 - 0x57fff, 2 GiB
[0.040957] NOTICE: BL2: DDR3200(rev.0.40)
[0.052413] NOTICE: BL2: [COLD_BOOT]
[0.058356] NOTICE: BL2: DRAM Split is 2ch(DDR 3)
[0.061655] NOTICE: BL2: QoS is default setting(rev.0.07)
[0.067098] NOTICE: BL2: DRAM refresh interval 1.95 usec
[0.072455] NOTICE: BL2: Periodic Write DQ Training
[0.077485] NOTICE: BL2: Lossy Decomp areas
[0.081614] NOTICE: Entry 0: DCMPAREACRAx:0x8540 DCMPAREACRBx:0x570
[0.088698] NOTICE: Entry 1: DCMPAREACRAx:0x4000 DCMPAREACRBx:0x0
[0.095611] NOTICE: Entry 2: DCMPAREACRAx:0x2000 DCMPAREACRBx:0x0
[0.102525] NOTICE: BL2: v1.5(release):af9f429-dirty
[0.107534] NOTICE: BL2: Built : 13:06:47, Jun 11 2020
[0.112722] NOTICE: BL2: Normal boot
[0.116360] NOTICE: BL2: dst=0xe6321100 src=0x818 len=512(0x200)
[0.122850] NOTICE: BL2: dst=0x43f0 src=0x8180400 len=6144(0x1800)
[0.130588] NOTICE: BL2: dst=0x4400 src=0x81c len=65536(0x1)
[0.149490] NOTICE: BL2: dst=0x5000 src=0x830 len=1048576(0x10)
[0.369751] NOTICE: BL2: Booting BL31
U-Boot 2018.09 (Jun 11 2020 - 13:06:59 +)
CPU: Renesas Electronics R8A774E1 rev 3.0
Model: Hoperun Technology HiHope RZ/G2H platform (hihope-rzg2h)
DRAM: 3.9 GiB
Bank #0: 0x04800 - 0x0bfff, 1.9 GiB
Bank #1: 0x5 - 0x57fff, 2 GiB
Watchdog: Not found by seq!
WDT: watchdog@e602
Watchdog: Started!
MMC: sd@ee10: 0, sd@ee16: 1
Loading Environment from MMC... OK
In:serial@e6e88000
Out: serial@e6e88000
Err: serial@e6e88000
Net: eth0: ethernet@e680
Hit any key to stop autoboot: 0
ethernet@e680 Waiting for PHY auto negotiation to complete... done
BOOTP broadcast 1
BOOTP broadcast 2
BOOTP broadcast 3
BOOTP broadcast 4
DHCP client bound to address 192.168.10.136 (1798 ms)
Using ethernet@e680 device
TFTP from server 192.168.10.1; our IP address is 192.168.10.136
Filename 'g2h/Image.gz'.
Load address: 0x4a08
Loading: T #
#
#
#
#
#
RE: [PATCH 2/9] iommu/ipmmu-vmsa: Hook up R8A774E1 DT matching code
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
> wrote:
> > On Tue, Jul 14, 2020 at 9:09 AM Geert Uytterhoeven
> > wrote:
> > > On Mon, Jul 13, 2020 at 11:35 PM Lad Prabhakar
> > > wrote:
> > > > From: Marian-Cristian Rotariu
> > > >
> > > >
> > > > Add support for RZ/G2H (R8A774E1) SoC IPMMUs.
> > > >
> > > > Signed-off-by: Marian-Cristian Rotariu
> > > >
> > > > Signed-off-by: Lad Prabhakar
> > >
> > > 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/9] iommu/ipmmu-vmsa: Hook up R8A774E1 DT matching code
Hi Shimoda-san,
On Tue, Jul 14, 2020 at 1:42 PM Yoshihiro Shimoda
wrote:
> > From: Geert Uytterhoeven, Sent: Tuesday, July 14, 2020 5:42 PM
> > On Tue, Jul 14, 2020 at 10:30 AM Lad, Prabhakar
> > wrote:
> > > On Tue, Jul 14, 2020 at 9:09 AM Geert Uytterhoeven
> > > 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 -- ge...@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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/9] iommu/ipmmu-vmsa: Hook up R8A774E1 DT matching code
Hi Prabhakar,
On Tue, Jul 14, 2020 at 10:30 AM Lad, Prabhakar
wrote:
> On Tue, Jul 14, 2020 at 9:09 AM Geert Uytterhoeven
> wrote:
> > On Mon, Jul 13, 2020 at 11:35 PM Lad Prabhakar
> > wrote:
> > > From: Marian-Cristian Rotariu
> > >
> > > Add support for RZ/G2H (R8A774E1) SoC IPMMUs.
> > >
> > > Signed-off-by: Marian-Cristian Rotariu
> > >
> > > Signed-off-by: Lad Prabhakar
> >
> > 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 -- ge...@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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/9] iommu/ipmmu-vmsa: Hook up R8A774E1 DT matching code
Hi Prabhakar,
On Mon, Jul 13, 2020 at 11:35 PM Lad Prabhakar
wrote:
> From: Marian-Cristian Rotariu
>
> Add support for RZ/G2H (R8A774E1) SoC IPMMUs.
>
> Signed-off-by: Marian-Cristian Rotariu
>
> Signed-off-by: Lad Prabhakar
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 -- ge...@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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
