| Message ID | c44545c6d07c65d89daa297298c27bb0f15c8b84.1728393458.git.robin.murphy@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | iommu/omap: Don't register ops by fwnode | expand |
Hi Robin, > Am 08.10.2024 um 15:17 schrieb Robin Murphy <robin.murphy@arm.com>: > > The OMAP driver still has its own traditional firmware parsing and > instance handling in omap_iommu_probe_device(), rather than using the > generic fwnode-based paths. However, it also passes a hwdev to > iommu_device_register(), thus registering a fwnode for each ops > instance, wherein __iommu_probe_device() then fails to find matching ops > for a client device with no fwspec and thus a NULL iommu_fwnode. > > Since omap-iommu is not known to coexist with any other IOMMU hardware > and shares the same ops between all instances, we can reasonably remove > the hwdev/fwnode registration to put it back into "legacy" mode where > the ops are effectively global and ->probe_device remains responsible > for filtering individual clients. > > Reported-by: Beleswar Padhi <b-padhi@ti.com> > Reported-by: H. Nikolaus Schaller <hns@goldelico.com> > Fixes: 17de3f5fdd35 ("iommu: Retire bus ops") > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/iommu/omap-iommu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c > index c9528065a59a..425ae8e551dc 100644 > --- a/drivers/iommu/omap-iommu.c > +++ b/drivers/iommu/omap-iommu.c > @@ -1230,7 +1230,7 @@ static int omap_iommu_probe(struct platform_device *pdev) > if (err) > return err; > > - err = iommu_device_register(&obj->iommu, &omap_iommu_ops, &pdev->dev); > + err = iommu_device_register(&obj->iommu, &omap_iommu_ops, NULL); Thanks for this proposal, but this prevents my OMAP3 system completely from booting (at least with v6.8-rc1): ## Booting kernel from Legacy Image at 82000000 ... Image Name: Linux-6.8.0-rc1-letux+ Image Type: ARM Linux Kernel Image (uncompressed) Data Size: 6491520 Bytes = 6.2 MiB Load Address: 80008000 Entry Point: 80008000 Verifying Checksum ... OK ## Flattened Device Tree blob at 81c00000 Booting using the fdt blob at 0x81c00000 Loading Kernel Image ... OK Using Device Tree in place at 81c00000, end 81c1fe8e Starting kernel ... --- no further reaction -- As far as I see, all relevant devices are in the iommu_device_list but only omap3.isp is really using iommu. So some other devices may fail by this change. BR, Nikolaus
On 08/10/2024 2:32 pm, H. Nikolaus Schaller wrote: > Hi Robin, > >> Am 08.10.2024 um 15:17 schrieb Robin Murphy <robin.murphy@arm.com>: >> >> The OMAP driver still has its own traditional firmware parsing and >> instance handling in omap_iommu_probe_device(), rather than using the >> generic fwnode-based paths. However, it also passes a hwdev to >> iommu_device_register(), thus registering a fwnode for each ops >> instance, wherein __iommu_probe_device() then fails to find matching ops >> for a client device with no fwspec and thus a NULL iommu_fwnode. >> >> Since omap-iommu is not known to coexist with any other IOMMU hardware >> and shares the same ops between all instances, we can reasonably remove >> the hwdev/fwnode registration to put it back into "legacy" mode where >> the ops are effectively global and ->probe_device remains responsible >> for filtering individual clients. >> >> Reported-by: Beleswar Padhi <b-padhi@ti.com> >> Reported-by: H. Nikolaus Schaller <hns@goldelico.com> >> Fixes: 17de3f5fdd35 ("iommu: Retire bus ops") >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> --- >> drivers/iommu/omap-iommu.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c >> index c9528065a59a..425ae8e551dc 100644 >> --- a/drivers/iommu/omap-iommu.c >> +++ b/drivers/iommu/omap-iommu.c >> @@ -1230,7 +1230,7 @@ static int omap_iommu_probe(struct platform_device *pdev) >> if (err) >> return err; >> >> - err = iommu_device_register(&obj->iommu, &omap_iommu_ops, &pdev->dev); >> + err = iommu_device_register(&obj->iommu, &omap_iommu_ops, NULL); > > Thanks for this proposal, but this prevents my OMAP3 system completely > from booting (at least with v6.8-rc1): > > ## Booting kernel from Legacy Image at 82000000 ... > Image Name: Linux-6.8.0-rc1-letux+ > Image Type: ARM Linux Kernel Image (uncompressed) > Data Size: 6491520 Bytes = 6.2 MiB > Load Address: 80008000 > Entry Point: 80008000 > Verifying Checksum ... OK > ## Flattened Device Tree blob at 81c00000 > Booting using the fdt blob at 0x81c00000 > Loading Kernel Image ... OK > Using Device Tree in place at 81c00000, end 81c1fe8e > > Starting kernel ... > > --- no further reaction -- Urgh... is it possible to get earlycon/ealyprintk output on this platform? As a stab in the dark by inspection, there might potentially be some interaction with "iommu: Move bus setup to IOMMU device registration" as well, for which the additional diff below might help... Thanks, Robin. ---->8---- diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c index 425ae8e551dc..9112178e22d8 100644 --- a/drivers/iommu/omap-iommu.c +++ b/drivers/iommu/omap-iommu.c @@ -1691,7 +1691,8 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev) if (!oiommu) { of_node_put(np); kfree(arch_data); - return ERR_PTR(-EINVAL); + /* Not fatal, will re-probe once the right instance registers itself */ + return ERR_PTR(-ENODEV); } tmp->iommu_dev = oiommu; > > As far as I see, all relevant devices are in the iommu_device_list but > only omap3.isp is really using iommu. So some other devices may fail by > this change. > > BR, > Nikolaus >
Hi Robin, > Am 08.10.2024 um 16:00 schrieb Robin Murphy <robin.murphy@arm.com>: > > On 08/10/2024 2:32 pm, H. Nikolaus Schaller wrote: >> Hi Robin, >>> Am 08.10.2024 um 15:17 schrieb Robin Murphy <robin.murphy@arm.com>: >>> >>> The OMAP driver still has its own traditional firmware parsing and >>> instance handling in omap_iommu_probe_device(), rather than using the >>> generic fwnode-based paths. However, it also passes a hwdev to >>> iommu_device_register(), thus registering a fwnode for each ops >>> instance, wherein __iommu_probe_device() then fails to find matching ops >>> for a client device with no fwspec and thus a NULL iommu_fwnode. >>> >>> Since omap-iommu is not known to coexist with any other IOMMU hardware >>> and shares the same ops between all instances, we can reasonably remove >>> the hwdev/fwnode registration to put it back into "legacy" mode where >>> the ops are effectively global and ->probe_device remains responsible >>> for filtering individual clients. >>> >>> Reported-by: Beleswar Padhi <b-padhi@ti.com> >>> Reported-by: H. Nikolaus Schaller <hns@goldelico.com> >>> Fixes: 17de3f5fdd35 ("iommu: Retire bus ops") >>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >>> --- >>> drivers/iommu/omap-iommu.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c >>> index c9528065a59a..425ae8e551dc 100644 >>> --- a/drivers/iommu/omap-iommu.c >>> +++ b/drivers/iommu/omap-iommu.c >>> @@ -1230,7 +1230,7 @@ static int omap_iommu_probe(struct platform_device *pdev) >>> if (err) >>> return err; >>> >>> - err = iommu_device_register(&obj->iommu, &omap_iommu_ops, &pdev->dev); >>> + err = iommu_device_register(&obj->iommu, &omap_iommu_ops, NULL); >> Thanks for this proposal, but this prevents my OMAP3 system completely >> from booting (at least with v6.8-rc1): >> ## Booting kernel from Legacy Image at 82000000 ... >> Image Name: Linux-6.8.0-rc1-letux+ >> Image Type: ARM Linux Kernel Image (uncompressed) >> Data Size: 6491520 Bytes = 6.2 MiB >> Load Address: 80008000 >> Entry Point: 80008000 >> Verifying Checksum ... OK >> ## Flattened Device Tree blob at 81c00000 >> Booting using the fdt blob at 0x81c00000 >> Loading Kernel Image ... OK >> Using Device Tree in place at 81c00000, end 81c1fe8e >> Starting kernel ... >> --- no further reaction -- > > Urgh... is it possible to get earlycon/ealyprintk output on this platform? Ah, my mistake. earlycon did reveal a NULL pointer dereference coming from that I have added some printk() to iommu_device_register() and friends. And one assumed that hwdev is not a NULL pointer and we can print hwdev->kobj.name... > > As a stab in the dark by inspection, there might potentially be some interaction with "iommu: Move bus setup to IOMMU device registration" as well, for which the additional diff below might help... > > Thanks, > Robin. > > ---->8---- > diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c > index 425ae8e551dc..9112178e22d8 100644 > --- a/drivers/iommu/omap-iommu.c > +++ b/drivers/iommu/omap-iommu.c > @@ -1691,7 +1691,8 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev) > if (!oiommu) { > of_node_put(np); > kfree(arch_data); > - return ERR_PTR(-EINVAL); > + /* Not fatal, will re-probe once the right instance registers itself */ > + return ERR_PTR(-ENODEV); > } > > tmp->iommu_dev = oiommu; Now I can boot with both patches applied (and Jason's patch and my printk removed), but there is still (exactly the same as with Jason's patch): root@letux:~# uname -a Linux letux 6.8.0-rc1-letux+ #19517 SMP PREEMPT Tue Oct 8 17:23:11 CEST 2024 armv7l GNU/Linux root@letux:~# dmesg|fgrep iommu [ 0.700195] iommu: Default domain type: Translated [ 0.705078] iommu: DMA domain TLB invalidation policy: strict mode [ 0.728393] platform 480bc000.isp: Adding to iommu group 0 [ 0.734161] omap-iommu 480bd400.mmu: 480bd400.mmu registered [ 23.565216] omap3isp 480bc000.isp: iommu configuration for device failed with -ETIMEDOUT [ 23.605102] omap-iommu 480bd400.mmu: 480bd400.mmu: version 1.1 root@letux:~# dmesg|fgrep isp [ 0.000000] OMAP3630/DM3730 ES1.2 (l2cache iva sgx neon isp 192mhz_clk) [ 0.728393] platform 480bc000.isp: Adding to iommu group 0 [ 11.472045] omapdss_dss 48050000.dss: bound 48050400.dispc (ops dsi_vc_flush_receive_data [omapdrm]) <-- false positive of fgrep [ 23.557586] omap3isp 480bc000.isp: deferred probe timeout, ignoring dependency [ 23.565216] omap3isp 480bc000.isp: iommu configuration for device failed with -ETIMEDOUT <-- duplicate with fgrep iommu [ 23.625183] omap3isp 480bc000.isp: supply vdd-csiphy1 not found, using dummy regulator [ 23.657135] omap3isp 480bc000.isp: supply vdd-csiphy2 not found, using dummy regulator [ 23.665832] omap3isp 480bc000.isp: Revision 15.0 found [ 23.700073] omap3isp 480bc000.isp: failed to attach device to VA mapping [ 23.724182] omap3isp 480bc000.isp: unable to attach to IOMMU [ 23.731262] omap3isp: probe of 480bc000.isp failed with error -16 root@letux:~# The -ETIMEDOUT seems to come from of_iommu_configure(). I also did now run the same with v6.7 to compare timing and message sequence: root@letux:~# uname -a Linux letux 6.7.0-letux+ #19518 SMP PREEMPT Tue Oct 8 17:48:27 CEST 2024 armv7l GNU/Linux root@letux:~# dmesg|fgrep iommu [ 0.412078] iommu: Default domain type: Translated [ 0.412109] iommu: DMA domain TLB invalidation policy: strict mode [ 0.415008] platform 480bc000.isp: Adding to iommu group 0 [ 0.415191] omap-iommu 480bd400.mmu: 480bd400.mmu registered [ 11.046630] omap-iommu 480bd400.mmu: 480bd400.mmu: version 1.1 root@letux:~# dmesg|fgrep isp [ 0.000000] OMAP3630/DM3730 ES1.2 (l2cache iva sgx neon isp 192mhz_clk) [ 0.415008] platform 480bc000.isp: Adding to iommu group 0 [ 10.885711] omap3isp 480bc000.isp: supply vdd-csiphy1 not found, using dummy regulator [ 10.953338] omap3isp 480bc000.isp: supply vdd-csiphy2 not found, using dummy regulator [ 11.025482] omap3isp 480bc000.isp: Revision 15.0 found [ 11.084014] omap3isp 480bc000.isp: hist: using DMA channel dma0chan15 [ 11.150695] omap3isp 480bc000.isp: Entity type for entity OMAP3 ISP CCP2 was not initialized! [ 11.162109] omap3isp 480bc000.isp: isp_xclk_set_rate: cam_xclka set to 24685714 Hz (div 7) [ 11.199523] omap3isp 480bc000.isp: Entity type for entity OMAP3 ISP CSI2a was not initialized! [ 11.291931] omap3isp 480bc000.isp: Entity type for entity OMAP3 ISP CCDC was not initialized! [ 11.321807] omap3isp 480bc000.isp: Entity type for entity OMAP3 ISP preview was not initialized! [ 11.393188] omap3isp 480bc000.isp: Entity type for entity OMAP3 ISP resizer was not initialized! [ 11.425109] omap3isp 480bc000.isp: Entity type for entity OMAP3 ISP AEWB was not initialized! [ 11.434082] omap3isp 480bc000.isp: Entity type for entity OMAP3 ISP AF was not initialized! [ 11.534820] omap3isp 480bc000.isp: Entity type for entity OMAP3 ISP histogram was not initialized! [ 11.565155] omap3isp 480bc000.isp: parsing parallel interface [ 12.589965] omapdss_dss 48050000.dss: bound 48050400.dispc (ops dsi_vc_flush_receive_data [omapdrm]) root@letux:~# Interestingly, there is no -ETIMEOUT and initialization start much earlier. BR and thanks, Nikolaus
On 08/10/2024 5:13 pm, H. Nikolaus Schaller wrote: > Hi Robin, > >> Am 08.10.2024 um 16:00 schrieb Robin Murphy <robin.murphy@arm.com>: >> >> On 08/10/2024 2:32 pm, H. Nikolaus Schaller wrote: >>> Hi Robin, >>>> Am 08.10.2024 um 15:17 schrieb Robin Murphy <robin.murphy@arm.com>: >>>> >>>> The OMAP driver still has its own traditional firmware parsing and >>>> instance handling in omap_iommu_probe_device(), rather than using the >>>> generic fwnode-based paths. However, it also passes a hwdev to >>>> iommu_device_register(), thus registering a fwnode for each ops >>>> instance, wherein __iommu_probe_device() then fails to find matching ops >>>> for a client device with no fwspec and thus a NULL iommu_fwnode. >>>> >>>> Since omap-iommu is not known to coexist with any other IOMMU hardware >>>> and shares the same ops between all instances, we can reasonably remove >>>> the hwdev/fwnode registration to put it back into "legacy" mode where >>>> the ops are effectively global and ->probe_device remains responsible >>>> for filtering individual clients. >>>> >>>> Reported-by: Beleswar Padhi <b-padhi@ti.com> >>>> Reported-by: H. Nikolaus Schaller <hns@goldelico.com> >>>> Fixes: 17de3f5fdd35 ("iommu: Retire bus ops") >>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >>>> --- >>>> drivers/iommu/omap-iommu.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c >>>> index c9528065a59a..425ae8e551dc 100644 >>>> --- a/drivers/iommu/omap-iommu.c >>>> +++ b/drivers/iommu/omap-iommu.c >>>> @@ -1230,7 +1230,7 @@ static int omap_iommu_probe(struct platform_device *pdev) >>>> if (err) >>>> return err; >>>> >>>> - err = iommu_device_register(&obj->iommu, &omap_iommu_ops, &pdev->dev); >>>> + err = iommu_device_register(&obj->iommu, &omap_iommu_ops, NULL); >>> Thanks for this proposal, but this prevents my OMAP3 system completely >>> from booting (at least with v6.8-rc1): >>> ## Booting kernel from Legacy Image at 82000000 ... >>> Image Name: Linux-6.8.0-rc1-letux+ >>> Image Type: ARM Linux Kernel Image (uncompressed) >>> Data Size: 6491520 Bytes = 6.2 MiB >>> Load Address: 80008000 >>> Entry Point: 80008000 >>> Verifying Checksum ... OK >>> ## Flattened Device Tree blob at 81c00000 >>> Booting using the fdt blob at 0x81c00000 >>> Loading Kernel Image ... OK >>> Using Device Tree in place at 81c00000, end 81c1fe8e >>> Starting kernel ... >>> --- no further reaction -- >> >> Urgh... is it possible to get earlycon/ealyprintk output on this platform? > > Ah, my mistake. earlycon did reveal a NULL pointer dereference coming from that > I have added some printk() to iommu_device_register() and friends. And one > assumed that hwdev is not a NULL pointer and we can print hwdev->kobj.name... > >> >> As a stab in the dark by inspection, there might potentially be some interaction with "iommu: Move bus setup to IOMMU device registration" as well, for which the additional diff below might help... >> >> Thanks, >> Robin. >> >> ---->8---- >> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c >> index 425ae8e551dc..9112178e22d8 100644 >> --- a/drivers/iommu/omap-iommu.c >> +++ b/drivers/iommu/omap-iommu.c >> @@ -1691,7 +1691,8 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev) >> if (!oiommu) { >> of_node_put(np); >> kfree(arch_data); >> - return ERR_PTR(-EINVAL); >> + /* Not fatal, will re-probe once the right instance registers itself */ >> + return ERR_PTR(-ENODEV); >> } >> >> tmp->iommu_dev = oiommu; > > Now I can boot with both patches applied (and Jason's patch and my printk removed), > but there is still (exactly the same as with Jason's patch): > > root@letux:~# uname -a > Linux letux 6.8.0-rc1-letux+ #19517 SMP PREEMPT Tue Oct 8 17:23:11 CEST 2024 armv7l GNU/Linux > root@letux:~# dmesg|fgrep iommu > [ 0.700195] iommu: Default domain type: Translated > [ 0.705078] iommu: DMA domain TLB invalidation policy: strict mode > [ 0.728393] platform 480bc000.isp: Adding to iommu group 0 > [ 0.734161] omap-iommu 480bd400.mmu: 480bd400.mmu registered > [ 23.565216] omap3isp 480bc000.isp: iommu configuration for device failed with -ETIMEDOUT > [ 23.605102] omap-iommu 480bd400.mmu: 480bd400.mmu: version 1.1 > root@letux:~# dmesg|fgrep isp > [ 0.000000] OMAP3630/DM3730 ES1.2 (l2cache iva sgx neon isp 192mhz_clk) > [ 0.728393] platform 480bc000.isp: Adding to iommu group 0 > [ 11.472045] omapdss_dss 48050000.dss: bound 48050400.dispc (ops dsi_vc_flush_receive_data [omapdrm]) <-- false positive of fgrep > [ 23.557586] omap3isp 480bc000.isp: deferred probe timeout, ignoring dependency > [ 23.565216] omap3isp 480bc000.isp: iommu configuration for device failed with -ETIMEDOUT <-- duplicate with fgrep iommu > [ 23.625183] omap3isp 480bc000.isp: supply vdd-csiphy1 not found, using dummy regulator > [ 23.657135] omap3isp 480bc000.isp: supply vdd-csiphy2 not found, using dummy regulator > [ 23.665832] omap3isp 480bc000.isp: Revision 15.0 found > [ 23.700073] omap3isp 480bc000.isp: failed to attach device to VA mapping > [ 23.724182] omap3isp 480bc000.isp: unable to attach to IOMMU > [ 23.731262] omap3isp: probe of 480bc000.isp failed with error -16 > root@letux:~# > > The -ETIMEDOUT seems to come from of_iommu_configure(). Oof, yeah, now we've wound up with the opposite problem that because it's the generic "iommus" binding, it gets as far as of_iommu_xlate() but now the NULL fwnode no longer matches the phandle target so that thinks it's waiting for an instance which hasn't registered yet :( OK, different track, does the diff below fare any better? (I've not written it up fully yet as the DRA7 special cases will need some more work still) Thanks, Robin. ----->8----- diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c index c9528065a59a..44e09d60e861 100644 --- a/drivers/iommu/omap-iommu.c +++ b/drivers/iommu/omap-iommu.c @@ -1723,12 +1723,19 @@ static void omap_iommu_release_device(struct device *dev) } +int omap_iommu_of_xlate(struct device *dev, const struct of_phandle_args *args) +{ + /* TODO: collect args->np to save re-parsing in probe above */ + return 0; +} + static const struct iommu_ops omap_iommu_ops = { .identity_domain = &omap_iommu_identity_domain, .domain_alloc_paging = omap_iommu_domain_alloc_paging, .probe_device = omap_iommu_probe_device, .release_device = omap_iommu_release_device, .device_group = generic_single_device_group, + .of_xlate = omap_iommu_of_xlate, .pgsize_bitmap = OMAP_IOMMU_PGSIZES, .default_domain_ops = &(const struct iommu_domain_ops) { .attach_dev = omap_iommu_attach_dev,
> Am 08.10.2024 um 20:20 schrieb Robin Murphy <robin.murphy@arm.com>: >> The -ETIMEDOUT seems to come from of_iommu_configure(). > > Oof, yeah, now we've wound up with the opposite problem that because it's the generic "iommus" binding, it gets as far as of_iommu_xlate() but now the NULL fwnode no longer matches the phandle target so that thinks it's waiting for an instance which hasn't registered yet :( > > OK, different track, does the diff below fare any better? (I've not written it up fully yet as the DRA7 special cases will need some more work still) > > Thanks, > Robin. > > ----->8----- > > diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c > index c9528065a59a..44e09d60e861 100644 > --- a/drivers/iommu/omap-iommu.c > +++ b/drivers/iommu/omap-iommu.c > @@ -1723,12 +1723,19 @@ static void omap_iommu_release_device(struct device *dev) > > } > > +int omap_iommu_of_xlate(struct device *dev, const struct of_phandle_args *args) > +{ > + /* TODO: collect args->np to save re-parsing in probe above */ > + return 0; > +} > + > static const struct iommu_ops omap_iommu_ops = { > .identity_domain = &omap_iommu_identity_domain, > .domain_alloc_paging = omap_iommu_domain_alloc_paging, > .probe_device = omap_iommu_probe_device, > .release_device = omap_iommu_release_device, > .device_group = generic_single_device_group, > + .of_xlate = omap_iommu_of_xlate, > .pgsize_bitmap = OMAP_IOMMU_PGSIZES, > .default_domain_ops = &(const struct iommu_domain_ops) { > .attach_dev = omap_iommu_attach_dev, Unfortunately no change :( A very tiny issue was that the second argument can not have a const specifier in the v6.8 series, but starting with v6.9 it should be there. But since 6.8 and 6.9 are already EOL, there will be no back-ports anyways. And if someone does really backport (like me for testing purposes) it is obvious what to do. BR and thanks, Nikolaus
On 08/10/2024 8:52 pm, H. Nikolaus Schaller wrote: > > >> Am 08.10.2024 um 20:20 schrieb Robin Murphy <robin.murphy@arm.com>: > >>> The -ETIMEDOUT seems to come from of_iommu_configure(). >> >> Oof, yeah, now we've wound up with the opposite problem that because it's the generic "iommus" binding, it gets as far as of_iommu_xlate() but now the NULL fwnode no longer matches the phandle target so that thinks it's waiting for an instance which hasn't registered yet :( >> >> OK, different track, does the diff below fare any better? (I've not written it up fully yet as the DRA7 special cases will need some more work still) >> >> Thanks, >> Robin. >> >> ----->8----- >> >> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c >> index c9528065a59a..44e09d60e861 100644 >> --- a/drivers/iommu/omap-iommu.c >> +++ b/drivers/iommu/omap-iommu.c >> @@ -1723,12 +1723,19 @@ static void omap_iommu_release_device(struct device *dev) >> >> } >> >> +int omap_iommu_of_xlate(struct device *dev, const struct of_phandle_args *args) >> +{ >> + /* TODO: collect args->np to save re-parsing in probe above */ >> + return 0; >> +} >> + >> static const struct iommu_ops omap_iommu_ops = { >> .identity_domain = &omap_iommu_identity_domain, >> .domain_alloc_paging = omap_iommu_domain_alloc_paging, >> .probe_device = omap_iommu_probe_device, >> .release_device = omap_iommu_release_device, >> .device_group = generic_single_device_group, >> + .of_xlate = omap_iommu_of_xlate, >> .pgsize_bitmap = OMAP_IOMMU_PGSIZES, >> .default_domain_ops = &(const struct iommu_domain_ops) { >> .attach_dev = omap_iommu_attach_dev, > > > Unfortunately no change :( Hmm, I dug around and found a Pandaboard in the cupboard, and ostensibly this seems to work as expected there: 6:12-rc3: chu-chu-rocket:~ # dmesg | grep -i iommu [ 0.628601] iommu: Default domain type: Translated [ 0.633575] iommu: DMA domain TLB invalidation policy: strict mode [ 1.636047] omap-iommu 4a066000.mmu: 4a066000.mmu registered [ 3.265869] omap-iommu 55082000.mmu: 55082000.mmu registered 6.12-rc3 + of_xlate: chu-chu-rocket:~ # dmesg | grep -i iommu [ 0.629577] iommu: Default domain type: Translated [ 0.634582] iommu: DMA domain TLB invalidation policy: strict mode [ 1.622802] omap-iommu 4a066000.mmu: 4a066000.mmu registered [ 3.316009] omap-iommu 55082000.mmu: 55082000.mmu registered [ 3.329040] omap-rproc ocp:dsp: Adding to iommu group 0 [ 3.335083] omap-iommu 4a066000.mmu: 4a066000.mmu: version 2.0 [ 3.356506] omap-rproc 55020000.ipu: Adding to iommu group 1 [ 3.362396] omap-iommu 55082000.mmu: 55082000.mmu: version 2.1 Guess I'm going to have to dig further for an OMAP3 to figure out what additional shenanigans that ISP driver is up to... :/ Thanks, Robin. > > A very tiny issue was that the second argument can not have a const specifier in the > v6.8 series, but starting with v6.9 it should be there. But since 6.8 and 6.9 are > already EOL, there will be no back-ports anyways. And if someone does really > backport (like me for testing purposes) it is obvious what to do. > > BR and thanks, > Nikolaus >
Hi Robin, > Am 25.10.2024 um 16:27 schrieb Robin Murphy <robin.murphy@arm.com>: > > On 08/10/2024 8:52 pm, H. Nikolaus Schaller wrote: >>> Am 08.10.2024 um 20:20 schrieb Robin Murphy <robin.murphy@arm.com>: >>>> The -ETIMEDOUT seems to come from of_iommu_configure(). >>> >>> Oof, yeah, now we've wound up with the opposite problem that because it's the generic "iommus" binding, it gets as far as of_iommu_xlate() but now the NULL fwnode no longer matches the phandle target so that thinks it's waiting for an instance which hasn't registered yet :( >>> >>> OK, different track, does the diff below fare any better? (I've not written it up fully yet as the DRA7 special cases will need some more work still) >>> >>> Thanks, >>> Robin. >>> >>> ----->8----- >>> >>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c >>> index c9528065a59a..44e09d60e861 100644 >>> --- a/drivers/iommu/omap-iommu.c >>> +++ b/drivers/iommu/omap-iommu.c >>> @@ -1723,12 +1723,19 @@ static void omap_iommu_release_device(struct device *dev) >>> >>> } >>> >>> +int omap_iommu_of_xlate(struct device *dev, const struct of_phandle_args *args) >>> +{ >>> + /* TODO: collect args->np to save re-parsing in probe above */ >>> + return 0; >>> +} >>> + >>> static const struct iommu_ops omap_iommu_ops = { >>> .identity_domain = &omap_iommu_identity_domain, >>> .domain_alloc_paging = omap_iommu_domain_alloc_paging, >>> .probe_device = omap_iommu_probe_device, >>> .release_device = omap_iommu_release_device, >>> .device_group = generic_single_device_group, >>> + .of_xlate = omap_iommu_of_xlate, >>> .pgsize_bitmap = OMAP_IOMMU_PGSIZES, >>> .default_domain_ops = &(const struct iommu_domain_ops) { >>> .attach_dev = omap_iommu_attach_dev, >> Unfortunately no change :( > > Hmm, I dug around and found a Pandaboard in the cupboard, and ostensibly this seems to work as expected there: Oh, fine! I did not yet think about cross-checking with my PandaES (because I have no camera connected). > > 6:12-rc3: > chu-chu-rocket:~ # dmesg | grep -i iommu > [ 0.628601] iommu: Default domain type: Translated > [ 0.633575] iommu: DMA domain TLB invalidation policy: strict mode > [ 1.636047] omap-iommu 4a066000.mmu: 4a066000.mmu registered > [ 3.265869] omap-iommu 55082000.mmu: 55082000.mmu registered > > 6.12-rc3 + of_xlate: > chu-chu-rocket:~ # dmesg | grep -i iommu > [ 0.629577] iommu: Default domain type: Translated > [ 0.634582] iommu: DMA domain TLB invalidation policy: strict mode > [ 1.622802] omap-iommu 4a066000.mmu: 4a066000.mmu registered > [ 3.316009] omap-iommu 55082000.mmu: 55082000.mmu registered > [ 3.329040] omap-rproc ocp:dsp: Adding to iommu group 0 > [ 3.335083] omap-iommu 4a066000.mmu: 4a066000.mmu: version 2.0 > [ 3.356506] omap-rproc 55020000.ipu: Adding to iommu group 1 > [ 3.362396] omap-iommu 55082000.mmu: 55082000.mmu: version 2.1 > > Guess I'm going to have to dig further for an OMAP3 to figure out what additional shenanigans that ISP driver is up to... :/ I don't have much time for this issue at the moment but will try to get new insights. BR, Nikolaus
Hi Robin, > Am 25.10.2024 um 16:27 schrieb Robin Murphy <robin.murphy@arm.com>: > > On 08/10/2024 8:52 pm, H. Nikolaus Schaller wrote: >>> Am 08.10.2024 um 20:20 schrieb Robin Murphy <robin.murphy@arm.com>: >>>> The -ETIMEDOUT seems to come from of_iommu_configure(). >>> > > Hmm, I dug around and found a Pandaboard in the cupboard, and ostensibly this seems to work as expected there: > > 6:12-rc3: > chu-chu-rocket:~ # dmesg | grep -i iommu > [ 0.628601] iommu: Default domain type: Translated > [ 0.633575] iommu: DMA domain TLB invalidation policy: strict mode > [ 1.636047] omap-iommu 4a066000.mmu: 4a066000.mmu registered > [ 3.265869] omap-iommu 55082000.mmu: 55082000.mmu registered > > 6.12-rc3 + of_xlate: > chu-chu-rocket:~ # dmesg | grep -i iommu > [ 0.629577] iommu: Default domain type: Translated > [ 0.634582] iommu: DMA domain TLB invalidation policy: strict mode > [ 1.622802] omap-iommu 4a066000.mmu: 4a066000.mmu registered > [ 3.316009] omap-iommu 55082000.mmu: 55082000.mmu registered > [ 3.329040] omap-rproc ocp:dsp: Adding to iommu group 0 > [ 3.335083] omap-iommu 4a066000.mmu: 4a066000.mmu: version 2.0 > [ 3.356506] omap-rproc 55020000.ipu: Adding to iommu group 1 > [ 3.362396] omap-iommu 55082000.mmu: 55082000.mmu: version 2.1 > > Guess I'm going to have to dig further for an OMAP3 to figure out what additional shenanigans that ISP driver is up to... :/ I was able to confirm on 6.12-rc4 ecept a small difference in the last lines (ocp:dsp and no version 2.x): root@letux:~# dmesg|fgrep iommu [ 0.402191] iommu: Default domain type: Translated [ 0.402221] iommu: DMA domain TLB invalidation policy: strict mode [ 0.741607] omap-iommu 4a066000.mmu: 4a066000.mmu registered [ 2.461120] omap-iommu 55082000.mmu: 55082000.mmu registered [ 2.474914] platform ocp:dsp: Adding to iommu group 0 [ 2.481719] platform 55020000.ipu: Adding to iommu group 1 root@letux:~# On OMAP3 it is similar. The patch is doing something. I get this w/o patch: root@letux:~# dmesg|fgrep iommu [ 0.515808] iommu: Default domain type: Translated [ 0.515808] iommu: DMA domain TLB invalidation policy: strict mode [ 0.528686] omap-iommu 480bd400.mmu: 480bd400.mmu registered root@letux:~# dmesg|fgrep .isp [ 10.924438] omap3isp 480bc000.isp: supply vdd-csiphy1 not found, using dummy regulator [ 10.989044] omap3isp 480bc000.isp: supply vdd-csiphy2 not found, using dummy regulator [ 11.082916] omap3isp 480bc000.isp: Revision 15.0 found [ 11.107879] omap3isp 480bc000.isp: failed to create ARM IOMMU mapping [ 11.160736] omap3isp 480bc000.isp: unable to attach to IOMMU [ 11.181640] omap3isp 480bc000.isp: isp_xclk_set_rate: cam_xclka set to 24685714 Hz (div 7) root@letux:~# with patch: root@letux:~# dmesg|fgrep iommu [ 0.522552] iommu: Default domain type: Translated [ 0.522583] iommu: DMA domain TLB invalidation policy: strict mode [ 0.525543] platform 480bc000.isp: Adding to iommu group 0 [ 0.525726] omap-iommu 480bd400.mmu: 480bd400.mmu registered [ 34.422973] omap-iommu 480bd400.mmu: 480bd400.mmu: version 1.1 root@letux:~# dmesg|fgrep .isp [ 0.525543] platform 480bc000.isp: Adding to iommu group 0 [ 34.413330] omap3isp 480bc000.isp: deferred probe timeout, ignoring dependency [ 34.429412] omap3isp 480bc000.isp: supply vdd-csiphy1 not found, using dummy regulator [ 34.446655] omap3isp 480bc000.isp: supply vdd-csiphy2 not found, using dummy regulator [ 34.456115] omap3isp 480bc000.isp: Revision 15.0 found [ 34.463165] omap3isp 480bc000.isp: failed to attach device to VA mapping [ 34.470306] omap3isp 480bc000.isp: unable to attach to IOMMU [ 34.477661] omap3isp 480bc000.isp: probe with driver omap3isp failed with error -16 root@letux:~# Maybe I am missing something from your latest patches. I currently have: diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c index c9528065a59af..de14a3cd4b426 100644 --- a/drivers/iommu/omap-iommu.c +++ b/drivers/iommu/omap-iommu.c @@ -1230,7 +1230,8 @@ static int omap_iommu_probe(struct platform_device *pdev) if (err) return err; - err = iommu_device_register(&obj->iommu, &omap_iommu_ops, &pdev->dev); + err = iommu_device_register(&obj->iommu, &omap_iommu_ops, NULL); if (err) goto out_sysfs; obj->has_iommu_driver = true; @@ -1691,7 +1692,8 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev) if (!oiommu) { of_node_put(np); kfree(arch_data); - return ERR_PTR(-EINVAL); + /* Not fatal, will re-probe once the right instance registers itself */ + return ERR_PTR(-ENODEV); } tmp->iommu_dev = oiommu; Do you have a link to the patches you have tested? What I have also compared are the DTS and DTSI files of omap3/36xx and omap4/omap4-l4. Although there are architectural differences I didn't find anything significant. More different is how the omap3-isp vs. omap4-iva is handled. omap3-isp is a media driver calling arm_iommu_create_mapping() which fails. omap4-iva is a remoteproc driver using a different strategy (omap_rproc_of_get_internal_memories ?) where I do not know details where iommu comes into the game. Hope this helps. Best regards, Nikolaus
On 27/10/2024 5:15 pm, H. Nikolaus Schaller wrote: > Hi Robin, > >> Am 25.10.2024 um 16:27 schrieb Robin Murphy <robin.murphy@arm.com>: >> >> On 08/10/2024 8:52 pm, H. Nikolaus Schaller wrote: >>>> Am 08.10.2024 um 20:20 schrieb Robin Murphy <robin.murphy@arm.com>: >>>>> The -ETIMEDOUT seems to come from of_iommu_configure(). >>>> >> >> Hmm, I dug around and found a Pandaboard in the cupboard, and ostensibly this seems to work as expected there: >> >> 6:12-rc3: >> chu-chu-rocket:~ # dmesg | grep -i iommu >> [ 0.628601] iommu: Default domain type: Translated >> [ 0.633575] iommu: DMA domain TLB invalidation policy: strict mode >> [ 1.636047] omap-iommu 4a066000.mmu: 4a066000.mmu registered >> [ 3.265869] omap-iommu 55082000.mmu: 55082000.mmu registered >> >> 6.12-rc3 + of_xlate: >> chu-chu-rocket:~ # dmesg | grep -i iommu >> [ 0.629577] iommu: Default domain type: Translated >> [ 0.634582] iommu: DMA domain TLB invalidation policy: strict mode >> [ 1.622802] omap-iommu 4a066000.mmu: 4a066000.mmu registered >> [ 3.316009] omap-iommu 55082000.mmu: 55082000.mmu registered >> [ 3.329040] omap-rproc ocp:dsp: Adding to iommu group 0 >> [ 3.335083] omap-iommu 4a066000.mmu: 4a066000.mmu: version 2.0 >> [ 3.356506] omap-rproc 55020000.ipu: Adding to iommu group 1 >> [ 3.362396] omap-iommu 55082000.mmu: 55082000.mmu: version 2.1 >> >> Guess I'm going to have to dig further for an OMAP3 to figure out what additional shenanigans that ISP driver is up to... :/ > > I was able to confirm on 6.12-rc4 ecept a small difference in the last lines (ocp:dsp and no version 2.x): > > root@letux:~# dmesg|fgrep iommu > [ 0.402191] iommu: Default domain type: Translated > [ 0.402221] iommu: DMA domain TLB invalidation policy: strict mode > [ 0.741607] omap-iommu 4a066000.mmu: 4a066000.mmu registered > [ 2.461120] omap-iommu 55082000.mmu: 55082000.mmu registered > [ 2.474914] platform ocp:dsp: Adding to iommu group 0 > [ 2.481719] platform 55020000.ipu: Adding to iommu group 1 > root@letux:~# > > On OMAP3 it is similar. The patch is doing something. > > I get this w/o patch: > > root@letux:~# dmesg|fgrep iommu > [ 0.515808] iommu: Default domain type: Translated > [ 0.515808] iommu: DMA domain TLB invalidation policy: strict mode > [ 0.528686] omap-iommu 480bd400.mmu: 480bd400.mmu registered > root@letux:~# dmesg|fgrep .isp > [ 10.924438] omap3isp 480bc000.isp: supply vdd-csiphy1 not found, using dummy regulator > [ 10.989044] omap3isp 480bc000.isp: supply vdd-csiphy2 not found, using dummy regulator > [ 11.082916] omap3isp 480bc000.isp: Revision 15.0 found > [ 11.107879] omap3isp 480bc000.isp: failed to create ARM IOMMU mapping > [ 11.160736] omap3isp 480bc000.isp: unable to attach to IOMMU > [ 11.181640] omap3isp 480bc000.isp: isp_xclk_set_rate: cam_xclka set to 24685714 Hz (div 7) > root@letux:~# > > with patch: > > root@letux:~# dmesg|fgrep iommu > [ 0.522552] iommu: Default domain type: Translated > [ 0.522583] iommu: DMA domain TLB invalidation policy: strict mode > [ 0.525543] platform 480bc000.isp: Adding to iommu group 0 > [ 0.525726] omap-iommu 480bd400.mmu: 480bd400.mmu registered > [ 34.422973] omap-iommu 480bd400.mmu: 480bd400.mmu: version 1.1 > root@letux:~# dmesg|fgrep .isp > [ 0.525543] platform 480bc000.isp: Adding to iommu group 0 > [ 34.413330] omap3isp 480bc000.isp: deferred probe timeout, ignoring dependency > [ 34.429412] omap3isp 480bc000.isp: supply vdd-csiphy1 not found, using dummy regulator > [ 34.446655] omap3isp 480bc000.isp: supply vdd-csiphy2 not found, using dummy regulator > [ 34.456115] omap3isp 480bc000.isp: Revision 15.0 found > [ 34.463165] omap3isp 480bc000.isp: failed to attach device to VA mapping Oh fiddle... this is getting complex... I was testing the of_xlate patch rather than the diff below, which should avoid the deferred probe timeout but otherwise achieve much the same effect. Trouble is, that turns out to only "fix" things back to another breakage due to 4720287c7bf7 ("iommu: Remove struct iommu_ops *iommu from arch_setup_dma_ops()"), wherein we lost the fragile combination of setting up the IOMMU while still causing arm_setup_iommu_dma_ops() to ignore it. Thus what now happens here is the domain from the ARM DMA ops gets in the way of the driver attaching its own. Looks like I missed that with the remoteproc driver since it only starts trying to do IOMMU API stuff upon successfully loading firmware, which I didn't have. I think the most practical answer is going to be to give the ISP and remoteproc drivers the same workaround for that as the various DRM drivers use, lemme write those up too... Thanks, Robin. > [ 34.470306] omap3isp 480bc000.isp: unable to attach to IOMMU > [ 34.477661] omap3isp 480bc000.isp: probe with driver omap3isp failed with error -16 > root@letux:~# > > Maybe I am missing something from your latest patches. I currently have: > > diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c > index c9528065a59af..de14a3cd4b426 100644 > --- a/drivers/iommu/omap-iommu.c > +++ b/drivers/iommu/omap-iommu.c > @@ -1230,7 +1230,8 @@ static int omap_iommu_probe(struct platform_device *pdev) > if (err) > return err; > - err = iommu_device_register(&obj->iommu, &omap_iommu_ops, &pdev->dev); > + err = iommu_device_register(&obj->iommu, &omap_iommu_ops, NULL); > if (err) > goto out_sysfs; > obj->has_iommu_driver = true; > @@ -1691,7 +1692,8 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev) > if (!oiommu) { > of_node_put(np); > kfree(arch_data); > - return ERR_PTR(-EINVAL); > + /* Not fatal, will re-probe once the right instance registers itself */ > + return ERR_PTR(-ENODEV); > } > tmp->iommu_dev = oiommu; > > Do you have a link to the patches you have tested? > > What I have also compared are the DTS and DTSI files of omap3/36xx and omap4/omap4-l4. > Although there are architectural differences I didn't find anything significant. > More different is how the omap3-isp vs. omap4-iva is handled. > > omap3-isp is a media driver calling arm_iommu_create_mapping() which fails. > omap4-iva is a remoteproc driver using a different strategy (omap_rproc_of_get_internal_memories ?) > where I do not know details where iommu comes into the game. > > Hope this helps. > > Best regards, > Nikolaus >
> Am 28.10.2024 um 16:58 schrieb Robin Murphy <robin.murphy@arm.com>: > > > Oh fiddle... this is getting complex... > > I was testing the of_xlate patch rather than the diff below, which should avoid the deferred probe timeout but otherwise achieve much the same effect. Trouble is, that turns out to only "fix" things back to another breakage due to 4720287c7bf7 ("iommu: Remove struct iommu_ops *iommu from arch_setup_dma_ops()"), wherein we lost the fragile combination of setting up the IOMMU while still causing arm_setup_iommu_dma_ops() to ignore it. Thus what now happens here is the domain from the ARM DMA ops gets in the way of the driver attaching its own. Looks like I missed that with the remoteproc driver since it only starts trying to do IOMMU API stuff upon successfully loading firmware, which I didn't have. > > I think the most practical answer is going to be to give the ISP and remoteproc drivers the same workaround for that as the various DRM drivers use, lemme write those up too... Sounds good. Please let me know if you have something I can test. Best regards and thanks, Nikolaus
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c index c9528065a59a..425ae8e551dc 100644 --- a/drivers/iommu/omap-iommu.c +++ b/drivers/iommu/omap-iommu.c @@ -1230,7 +1230,7 @@ static int omap_iommu_probe(struct platform_device *pdev) if (err) return err; - err = iommu_device_register(&obj->iommu, &omap_iommu_ops, &pdev->dev); + err = iommu_device_register(&obj->iommu, &omap_iommu_ops, NULL); if (err) goto out_sysfs; obj->has_iommu_driver = true;
The OMAP driver still has its own traditional firmware parsing and instance handling in omap_iommu_probe_device(), rather than using the generic fwnode-based paths. However, it also passes a hwdev to iommu_device_register(), thus registering a fwnode for each ops instance, wherein __iommu_probe_device() then fails to find matching ops for a client device with no fwspec and thus a NULL iommu_fwnode. Since omap-iommu is not known to coexist with any other IOMMU hardware and shares the same ops between all instances, we can reasonably remove the hwdev/fwnode registration to put it back into "legacy" mode where the ops are effectively global and ->probe_device remains responsible for filtering individual clients. Reported-by: Beleswar Padhi <b-padhi@ti.com> Reported-by: H. Nikolaus Schaller <hns@goldelico.com> Fixes: 17de3f5fdd35 ("iommu: Retire bus ops") Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/iommu/omap-iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)