| Message ID | c44545c6d07c65d89daa297298c27bb0f15c8b84.1728393458.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| 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
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(-)