| Message ID | 20220302211100.65264-2-paul.kocialkowski@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allwinner A31/A83T MIPI CSI-2 and A31 ISP / Platform Support | expand |
+Saravana On Wed, Mar 02, 2022 at 10:10:53PM +0100, Paul Kocialkowski wrote: > In order to set their correct DMA address offset, some devices rely on > the device-tree interconnects property which identifies an > interconnect node that provides a dma-ranges property that can be used > to set said offset. > > Since that logic is all handled by the generic openfirmware and driver > code, the device-tree description could be enough to properly set > the offset. > > However the interconnects property is currently not marked as > optional, which implies that a driver for the corresponding node > must be loaded as a requirement. When no such driver exists, this > results in an endless EPROBE_DEFER which gets propagated to the > calling driver. This ends up in the driver never loading. > > Marking the interconnects property as optional makes it possible > to load the driver in that situation, since the EPROBE_DEFER return > code will no longer be propagated to the driver. > > There might however be undesirable consequences with this change, > which I do not fully grasp at this point. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > --- > drivers/of/property.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 8e90071de6ed..ef7c56b510e8 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -1365,7 +1365,7 @@ static struct device_node *parse_interrupts(struct device_node *np, > > static const struct supplier_bindings of_supplier_bindings[] = { > { .parse_prop = parse_clocks, }, > - { .parse_prop = parse_interconnects, }, > + { .parse_prop = parse_interconnects, .optional = true,}, > { .parse_prop = parse_iommus, .optional = true, }, > { .parse_prop = parse_iommu_maps, .optional = true, }, > { .parse_prop = parse_mboxes, }, > -- > 2.35.1 > >
On Mon, Mar 7, 2022 at 3:21 PM Rob Herring <robh@kernel.org> wrote: > > +Saravana > > On Wed, Mar 02, 2022 at 10:10:53PM +0100, Paul Kocialkowski wrote: > > In order to set their correct DMA address offset, some devices rely on > > the device-tree interconnects property which identifies an > > interconnect node that provides a dma-ranges property that can be used > > to set said offset. > > > > Since that logic is all handled by the generic openfirmware and driver > > code, the device-tree description could be enough to properly set > > the offset. > > > > However the interconnects property is currently not marked as > > optional, which implies that a driver for the corresponding node > > must be loaded as a requirement. When no such driver exists, this > > results in an endless EPROBE_DEFER which gets propagated to the > > calling driver. This ends up in the driver never loading. > > > > Marking the interconnects property as optional makes it possible > > to load the driver in that situation, since the EPROBE_DEFER return > > code will no longer be propagated to the driver. > > > > There might however be undesirable consequences with this change, > > which I do not fully grasp at this point. Temporary NACK till I get a bit more time to take a closer look. I really don't like the idea of making interconnects optional. IOMMUs and DMAs were exceptions. Also, we kinda discuss similar issues in LPC. We had some consensus on how to handle these and I noted them all down with a lot of details -- let me go take a look at those notes again and see if I can send a more generic patch. Paul, Can you point to the DTS (not DTSI) file that corresponds to this? Also, if it's a builtin kernel, I'd recommend setting deferred_probe_timeout=1 and that should take care of it too. -Saravana > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > --- > > drivers/of/property.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > index 8e90071de6ed..ef7c56b510e8 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -1365,7 +1365,7 @@ static struct device_node *parse_interrupts(struct device_node *np, > > > > static const struct supplier_bindings of_supplier_bindings[] = { > > { .parse_prop = parse_clocks, }, > > - { .parse_prop = parse_interconnects, }, > > + { .parse_prop = parse_interconnects, .optional = true,}, > > { .parse_prop = parse_iommus, .optional = true, }, > > { .parse_prop = parse_iommu_maps, .optional = true, }, > > { .parse_prop = parse_mboxes, }, > > -- > > 2.35.1 > > > >
Hi, On Mon, Mar 07, 2022 at 07:34:22PM -0800, Saravana Kannan wrote: > On Mon, Mar 7, 2022 at 3:21 PM Rob Herring <robh@kernel.org> wrote: > > > > +Saravana > > > > On Wed, Mar 02, 2022 at 10:10:53PM +0100, Paul Kocialkowski wrote: > > > In order to set their correct DMA address offset, some devices rely on > > > the device-tree interconnects property which identifies an > > > interconnect node that provides a dma-ranges property that can be used > > > to set said offset. > > > > > > Since that logic is all handled by the generic openfirmware and driver > > > code, the device-tree description could be enough to properly set > > > the offset. > > > > > > However the interconnects property is currently not marked as > > > optional, which implies that a driver for the corresponding node > > > must be loaded as a requirement. When no such driver exists, this > > > results in an endless EPROBE_DEFER which gets propagated to the > > > calling driver. This ends up in the driver never loading. > > > > > > Marking the interconnects property as optional makes it possible > > > to load the driver in that situation, since the EPROBE_DEFER return > > > code will no longer be propagated to the driver. > > > > > > There might however be undesirable consequences with this change, > > > which I do not fully grasp at this point. > > Temporary NACK till I get a bit more time to take a closer look. I > really don't like the idea of making interconnects optional. IOMMUs > and DMAs were exceptions. Also, we kinda discuss similar issues in > LPC. We had some consensus on how to handle these and I noted them all > down with a lot of details -- let me go take a look at those notes > again and see if I can send a more generic patch. > > Paul, > > Can you point to the DTS (not DTSI) file that corresponds to this? > Also, if it's a builtin kernel, I'd recommend setting > deferred_probe_timeout=1 and that should take care of it too. For the record, I also encountered this today on next-20220726 with this device: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun5i.dtsi#n775 The driver won't probe without fw_devlink=off Maxime
On Wed, Jul 27, 2022 at 5:06 AM Maxime Ripard <maxime@cerno.tech> wrote: > > Hi, > > On Mon, Mar 07, 2022 at 07:34:22PM -0800, Saravana Kannan wrote: > > On Mon, Mar 7, 2022 at 3:21 PM Rob Herring <robh@kernel.org> wrote: > > > > > > +Saravana > > > > > > On Wed, Mar 02, 2022 at 10:10:53PM +0100, Paul Kocialkowski wrote: > > > > In order to set their correct DMA address offset, some devices rely on > > > > the device-tree interconnects property which identifies an > > > > interconnect node that provides a dma-ranges property that can be used > > > > to set said offset. > > > > > > > > Since that logic is all handled by the generic openfirmware and driver > > > > code, the device-tree description could be enough to properly set > > > > the offset. > > > > > > > > However the interconnects property is currently not marked as > > > > optional, which implies that a driver for the corresponding node > > > > must be loaded as a requirement. When no such driver exists, this > > > > results in an endless EPROBE_DEFER which gets propagated to the > > > > calling driver. This ends up in the driver never loading. > > > > > > > > Marking the interconnects property as optional makes it possible > > > > to load the driver in that situation, since the EPROBE_DEFER return > > > > code will no longer be propagated to the driver. > > > > > > > > There might however be undesirable consequences with this change, > > > > which I do not fully grasp at this point. > > > > Temporary NACK till I get a bit more time to take a closer look. I > > really don't like the idea of making interconnects optional. IOMMUs > > and DMAs were exceptions. Also, we kinda discuss similar issues in > > LPC. We had some consensus on how to handle these and I noted them all > > down with a lot of details -- let me go take a look at those notes > > again and see if I can send a more generic patch. > > > > Paul, > > > > Can you point to the DTS (not DTSI) file that corresponds to this? > > Also, if it's a builtin kernel, I'd recommend setting > > deferred_probe_timeout=1 and that should take care of it too. > > For the record, I also encountered this today on next-20220726 with this > device: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun5i.dtsi#n775 > > The driver won't probe without fw_devlink=off Really? I basically ended up doing what I mentioned in my original reply. next-20220726 should have my changes that'll make sure fw_devlink doesn't block any probe (it'll still try to create as many device links as possible) after 10s (default deferred probe timeout). Can you try to find more info on why it's not probing? <debugfs>/devices_deferred should give more details. -Saravana
Hi, On Wed 27 Jul 22, 09:06, Saravana Kannan wrote: > On Wed, Jul 27, 2022 at 5:06 AM Maxime Ripard <maxime@cerno.tech> wrote: > > > > Hi, > > > > On Mon, Mar 07, 2022 at 07:34:22PM -0800, Saravana Kannan wrote: > > > On Mon, Mar 7, 2022 at 3:21 PM Rob Herring <robh@kernel.org> wrote: > > > > > > > > +Saravana > > > > > > > > On Wed, Mar 02, 2022 at 10:10:53PM +0100, Paul Kocialkowski wrote: > > > > > In order to set their correct DMA address offset, some devices rely on > > > > > the device-tree interconnects property which identifies an > > > > > interconnect node that provides a dma-ranges property that can be used > > > > > to set said offset. > > > > > > > > > > Since that logic is all handled by the generic openfirmware and driver > > > > > code, the device-tree description could be enough to properly set > > > > > the offset. > > > > > > > > > > However the interconnects property is currently not marked as > > > > > optional, which implies that a driver for the corresponding node > > > > > must be loaded as a requirement. When no such driver exists, this > > > > > results in an endless EPROBE_DEFER which gets propagated to the > > > > > calling driver. This ends up in the driver never loading. > > > > > > > > > > Marking the interconnects property as optional makes it possible > > > > > to load the driver in that situation, since the EPROBE_DEFER return > > > > > code will no longer be propagated to the driver. > > > > > > > > > > There might however be undesirable consequences with this change, > > > > > which I do not fully grasp at this point. > > > > > > Temporary NACK till I get a bit more time to take a closer look. I > > > really don't like the idea of making interconnects optional. IOMMUs > > > and DMAs were exceptions. Also, we kinda discuss similar issues in > > > LPC. We had some consensus on how to handle these and I noted them all > > > down with a lot of details -- let me go take a look at those notes > > > again and see if I can send a more generic patch. > > > > > > Paul, > > > > > > Can you point to the DTS (not DTSI) file that corresponds to this? > > > Also, if it's a builtin kernel, I'd recommend setting > > > deferred_probe_timeout=1 and that should take care of it too. > > > > For the record, I also encountered this today on next-20220726 with this > > device: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun5i.dtsi#n775 > > > > The driver won't probe without fw_devlink=off > > Really? I basically ended up doing what I mentioned in my original > reply. next-20220726 should have my changes that'll make sure > fw_devlink doesn't block any probe (it'll still try to create as many > device links as possible) after 10s (default deferred probe timeout). > Can you try to find more info on why it's not probing? > <debugfs>/devices_deferred should give more details. By the way last time I checked the initial issue that I reported appeared to be fixed by the patch (Extend deferred probe timeout on driver registration). Cheers, Paul
On Wed, Jul 27, 2022 at 10:17 AM Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > > Hi, > > On Wed 27 Jul 22, 09:06, Saravana Kannan wrote: > > On Wed, Jul 27, 2022 at 5:06 AM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > Hi, > > > > > > On Mon, Mar 07, 2022 at 07:34:22PM -0800, Saravana Kannan wrote: > > > > On Mon, Mar 7, 2022 at 3:21 PM Rob Herring <robh@kernel.org> wrote: > > > > > > > > > > +Saravana > > > > > > > > > > On Wed, Mar 02, 2022 at 10:10:53PM +0100, Paul Kocialkowski wrote: > > > > > > In order to set their correct DMA address offset, some devices rely on > > > > > > the device-tree interconnects property which identifies an > > > > > > interconnect node that provides a dma-ranges property that can be used > > > > > > to set said offset. > > > > > > > > > > > > Since that logic is all handled by the generic openfirmware and driver > > > > > > code, the device-tree description could be enough to properly set > > > > > > the offset. > > > > > > > > > > > > However the interconnects property is currently not marked as > > > > > > optional, which implies that a driver for the corresponding node > > > > > > must be loaded as a requirement. When no such driver exists, this > > > > > > results in an endless EPROBE_DEFER which gets propagated to the > > > > > > calling driver. This ends up in the driver never loading. > > > > > > > > > > > > Marking the interconnects property as optional makes it possible > > > > > > to load the driver in that situation, since the EPROBE_DEFER return > > > > > > code will no longer be propagated to the driver. > > > > > > > > > > > > There might however be undesirable consequences with this change, > > > > > > which I do not fully grasp at this point. > > > > > > > > Temporary NACK till I get a bit more time to take a closer look. I > > > > really don't like the idea of making interconnects optional. IOMMUs > > > > and DMAs were exceptions. Also, we kinda discuss similar issues in > > > > LPC. We had some consensus on how to handle these and I noted them all > > > > down with a lot of details -- let me go take a look at those notes > > > > again and see if I can send a more generic patch. > > > > > > > > Paul, > > > > > > > > Can you point to the DTS (not DTSI) file that corresponds to this? > > > > Also, if it's a builtin kernel, I'd recommend setting > > > > deferred_probe_timeout=1 and that should take care of it too. > > > > > > For the record, I also encountered this today on next-20220726 with this > > > device: > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun5i.dtsi#n775 > > > > > > The driver won't probe without fw_devlink=off > > > > Really? I basically ended up doing what I mentioned in my original > > reply. next-20220726 should have my changes that'll make sure > > fw_devlink doesn't block any probe (it'll still try to create as many > > device links as possible) after 10s (default deferred probe timeout). > > Can you try to find more info on why it's not probing? > > <debugfs>/devices_deferred should give more details. > > By the way last time I checked the initial issue that I reported appeared to be > fixed by the patch (Extend deferred probe timeout on driver registration). Thanks for the confirmation. -Saravana
diff --git a/drivers/of/property.c b/drivers/of/property.c index 8e90071de6ed..ef7c56b510e8 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -1365,7 +1365,7 @@ static struct device_node *parse_interrupts(struct device_node *np, static const struct supplier_bindings of_supplier_bindings[] = { { .parse_prop = parse_clocks, }, - { .parse_prop = parse_interconnects, }, + { .parse_prop = parse_interconnects, .optional = true,}, { .parse_prop = parse_iommus, .optional = true, }, { .parse_prop = parse_iommu_maps, .optional = true, }, { .parse_prop = parse_mboxes, },
In order to set their correct DMA address offset, some devices rely on the device-tree interconnects property which identifies an interconnect node that provides a dma-ranges property that can be used to set said offset. Since that logic is all handled by the generic openfirmware and driver code, the device-tree description could be enough to properly set the offset. However the interconnects property is currently not marked as optional, which implies that a driver for the corresponding node must be loaded as a requirement. When no such driver exists, this results in an endless EPROBE_DEFER which gets propagated to the calling driver. This ends up in the driver never loading. Marking the interconnects property as optional makes it possible to load the driver in that situation, since the EPROBE_DEFER return code will no longer be propagated to the driver. There might however be undesirable consequences with this change, which I do not fully grasp at this point. Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> --- drivers/of/property.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)