From: Philipp Zabel <p.zabel@pengutronix.de> Date: 2021-09-21 12:52:42
Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add
support for "resets" and "pwms""), the imx-drm driver fails to load
due to forever dormant devlinks to the reset-controller node. This
node is never associated with a struct device.
Drop those links, the reset controller is always available.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
arch/arm/mach-imx/src.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
Hi Philipp,
On Tue, Sep 21, 2021 at 9:51 AM Philipp Zabel [off-list ref] wrote:
Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add
support for "resets" and "pwms""), the imx-drm driver fails to load
due to forever dormant devlinks to the reset-controller node. This
node is never associated with a struct device.
Drop those links, the reset controller is always available.
On Tue, Sep 21, 2021 at 5:50 AM Philipp Zabel [off-list ref] wrote:
quoted hunk
Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add
support for "resets" and "pwms""), the imx-drm driver fails to load
due to forever dormant devlinks to the reset-controller node. This
node is never associated with a struct device.
Drop those links, the reset controller is always available.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
arch/arm/mach-imx/src.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
Hey Philipp,
Can you point me to the .dts file that corresponds to this platform? I
want to take a closer look.
-Saravana
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Philipp Zabel <p.zabel@pengutronix.de> Date: 2021-09-21 15:07:12
Hi Saravana,
On Tue, 2021-09-21 at 07:45 -0700, Saravana Kannan wrote:
On Tue, Sep 21, 2021 at 5:50 AM Philipp Zabel [off-list ref] wrote:
quoted
Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add
support for "resets" and "pwms""), the imx-drm driver fails to load
due to forever dormant devlinks to the reset-controller node. This
node is never associated with a struct device.
Drop those links, the reset controller is always available.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
arch/arm/mach-imx/src.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
Hey Philipp,
Can you point me to the .dts file that corresponds to this platform? I
want to take a closer look.
Thank you, the relevant file is arch/arm/boot/dts/imx6qdl.dtsi, device
nodes labeled as &src and &ipu1. This is for example included by
arch/arm/boot/dts/imx6q-nitrogen6x.dts. There's a second &ipu2 added in
arch/arm/boot/dts/imx6qd.dtsi as well.
I'm also seeing a few "Fixing up cyclic dependency with ldb/120000.hdmi"
messages, presumably due to the various of_graph links between ipu_di
and hdmi/ldb ports.
regards
Philipp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Philipp Zabel <p.zabel@pengutronix.de> Date: 2021-09-21 15:21:31
Hi Fabio,
On Tue, 2021-09-21 at 10:19 -0300, Fabio Estevam wrote:
Hi Philipp,
On Tue, Sep 21, 2021 at 9:51 AM Philipp Zabel [off-list ref] wrote:
quoted
Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add
support for "resets" and "pwms""), the imx-drm driver fails to load
due to forever dormant devlinks to the reset-controller node. This
node is never associated with a struct device.
Drop those links, the reset controller is always available.
This deserves a Fixes tag, so that it can land in 5.15.
Will do.
Tested on a imx6dl-pico-pi board and after applying your patch the
display controller is probed, thanks:
Tested-by: Fabio Estevam <festevam@gmail.com>
However, I get the following warning:
Thank you. This does not happen on HDMI-only boards, but on Nitrogen6X
with LVDS panel I see the same.
regards
Philipp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Sep 21, 2021 at 8:19 AM Philipp Zabel [off-list ref] wrote:
Hi Fabio,
On Tue, 2021-09-21 at 10:19 -0300, Fabio Estevam wrote:
quoted
Hi Philipp,
On Tue, Sep 21, 2021 at 9:51 AM Philipp Zabel [off-list ref] wrote:
quoted
Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add
support for "resets" and "pwms""), the imx-drm driver fails to load
due to forever dormant devlinks to the reset-controller node. This
node is never associated with a struct device.
Drop those links, the reset controller is always available.
This deserves a Fixes tag, so that it can land in 5.15.
Will do.
quoted
Tested on a imx6dl-pico-pi board and after applying your patch the
display controller is probed, thanks:
Tested-by: Fabio Estevam <festevam@gmail.com>
However, I get the following warning:
Thank you. This does not happen on HDMI-only boards, but on Nitrogen6X
with LVDS panel I see the same.
Let's hold off this patch for a day or two please. I want to take a
closer look to see if I have a more generic fix.
-Saravana
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Sep 21, 2021 at 8:05 AM Philipp Zabel [off-list ref] wrote:
Hi Saravana,
On Tue, 2021-09-21 at 07:45 -0700, Saravana Kannan wrote:
quoted
On Tue, Sep 21, 2021 at 5:50 AM Philipp Zabel [off-list ref] wrote:
quoted
Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add
support for "resets" and "pwms""), the imx-drm driver fails to load
due to forever dormant devlinks to the reset-controller node. This
node is never associated with a struct device.
Drop those links, the reset controller is always available.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
arch/arm/mach-imx/src.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
Hey Philipp,
Can you point me to the .dts file that corresponds to this platform? I
want to take a closer look.
Thank you, the relevant file is arch/arm/boot/dts/imx6qdl.dtsi, device
nodes labeled as &src and &ipu1. This is for example included by
arch/arm/boot/dts/imx6q-nitrogen6x.dts. There's a second &ipu2 added in
arch/arm/boot/dts/imx6qd.dtsi as well.
Thanks. I'll try to look at it within this week.
I'm also seeing a few "Fixing up cyclic dependency with ldb/120000.hdmi"
Those are fine. fw_devlink is just saying it's not enforcing probe
ordering between those devices because it sees a dependency cycle and
"gives up" trying to enforce probe ordering and lets the usual
deferred probe process take over.
-Saravana
messages, presumably due to the various of_graph links between ipu_di
and hdmi/ldb ports.
regards
Philipp
Hi Philipp,
On Tue, Sep 21, 2021 at 8:05 AM Philipp Zabel [off-list ref] wrote:
Hi Saravana,
On Tue, 2021-09-21 at 07:45 -0700, Saravana Kannan wrote:
quoted
On Tue, Sep 21, 2021 at 5:50 AM Philipp Zabel [off-list ref] wrote:
quoted
Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add
support for "resets" and "pwms""), the imx-drm driver fails to load
due to forever dormant devlinks to the reset-controller node. This
node is never associated with a struct device.
Drop those links, the reset controller is always available.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
arch/arm/mach-imx/src.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
Hey Philipp,
Can you point me to the .dts file that corresponds to this platform? I
want to take a closer look.
Thank you, the relevant file is arch/arm/boot/dts/imx6qdl.dtsi, device
nodes labeled as &src and &ipu1. This is for example included by
arch/arm/boot/dts/imx6q-nitrogen6x.dts. There's a second &ipu2 added in
arch/arm/boot/dts/imx6qd.dtsi as well.
I took a closer look at the DT. Is there really no struct device for
this node? Based on the DT, it looks like a device would automatically
get created for this node. So it looks like it's more of an issue with
the device not being probed by a real driver? Why can't you convert
the reset controller driver to a proper platform driver instead of
this patch? I think that's the right fix.
-Saravana
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Philipp Zabel <p.zabel@pengutronix.de> Date: 2021-09-22 14:15:28
Hi Saravana,
(Cc: Michael)
On Tue, 2021-09-21 at 09:47 -0700, Saravana Kannan wrote:
Hi Philipp,
On Tue, Sep 21, 2021 at 8:05 AM Philipp Zabel [off-list ref] wrote:
quoted
Hi Saravana,
On Tue, 2021-09-21 at 07:45 -0700, Saravana Kannan wrote:
quoted
On Tue, Sep 21, 2021 at 5:50 AM Philipp Zabel [off-list ref] wrote:
quoted
Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add
support for "resets" and "pwms""), the imx-drm driver fails to load
due to forever dormant devlinks to the reset-controller node. This
node is never associated with a struct device.
Drop those links, the reset controller is always available.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
arch/arm/mach-imx/src.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
Hey Philipp,
Can you point me to the .dts file that corresponds to this platform? I
want to take a closer look.
Thank you, the relevant file is arch/arm/boot/dts/imx6qdl.dtsi, device
nodes labeled as &src and &ipu1. This is for example included by
arch/arm/boot/dts/imx6q-nitrogen6x.dts. There's a second &ipu2 added in
arch/arm/boot/dts/imx6qd.dtsi as well.
I took a closer look at the DT. Is there really no struct device for
this node?
Yes, this node is handled by platform code in arch/arm/mach-imx/src.c as
the reset controller register also contains bits to bring up the CPUs.
Based on the DT, it looks like a device would automatically
get created for this node. So it looks like it's more of an issue with
the device not being probed by a real driver? Why can't you convert
the reset controller driver to a proper platform driver instead of
this patch? I think that's the right fix.
In this case, I could indeed add a platform driver for the reset
controller part. I'm not yet sure that this is always the right thing.
I just learned from Michael that there are other cases where fw_devlink
follows of_graph links from capture devices to connector nodes that have
a compatible, such as "composite-video-connector" or "svideo-connector",
but no associated driver. These are just parsed to determine the
connector type in drivers/media/v4l2-core/v4l2-fwnode.c. Would you
suggest to create platform devices for these as well?
regards
Philipp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Sep 22, 2021 at 7:12 AM Philipp Zabel [off-list ref] wrote:
Hi Saravana,
(Cc: Michael)
On Tue, 2021-09-21 at 09:47 -0700, Saravana Kannan wrote:
quoted
Hi Philipp,
On Tue, Sep 21, 2021 at 8:05 AM Philipp Zabel [off-list ref] wrote:
quoted
Hi Saravana,
On Tue, 2021-09-21 at 07:45 -0700, Saravana Kannan wrote:
quoted
On Tue, Sep 21, 2021 at 5:50 AM Philipp Zabel [off-list ref] wrote:
quoted
Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add
support for "resets" and "pwms""), the imx-drm driver fails to load
due to forever dormant devlinks to the reset-controller node. This
node is never associated with a struct device.
Drop those links, the reset controller is always available.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
arch/arm/mach-imx/src.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
Hey Philipp,
Can you point me to the .dts file that corresponds to this platform? I
want to take a closer look.
Thank you, the relevant file is arch/arm/boot/dts/imx6qdl.dtsi, device
nodes labeled as &src and &ipu1. This is for example included by
arch/arm/boot/dts/imx6q-nitrogen6x.dts. There's a second &ipu2 added in
arch/arm/boot/dts/imx6qd.dtsi as well.
I took a closer look at the DT. Is there really no struct device for
this node?
Yes, this node is handled by platform code in arch/arm/mach-imx/src.c as
Yes, I saw the node is handled in that file. I know it's not currently
a platform driver. I wasn't asking about that. I'm asking if a struct
device is created for the node or not. Creation of the struct device
is done automatically by the OF code and I don't see any code that
prevents the creation of the device.
Can you please run this on your device and that should answer my question"
# find /sys/devices/ -name *reset-controller*
I'm asking this question because there is a cleaner way to fix this
instead of what you are doing if there is actually a struct device
created for the node.
the reset controller register also contains bits to bring up the CPUs.
Ok, I initially assumed that's why you were doing this, but I didn't
see this &src node (the reset controller) being referenced anywhere
else in DT. So how does that work?
quoted
Based on the DT, it looks like a device would automatically
get created for this node. So it looks like it's more of an issue with
the device not being probed by a real driver? Why can't you convert
the reset controller driver to a proper platform driver instead of
this patch? I think that's the right fix.
In this case, I could indeed add a platform driver for the reset
controller part. I'm not yet sure that this is always the right thing.
Can you dig into that more please? Instead of just deleting the fwnode
links? There are also ways to make sure you don't block early users
(before driver core is up) while still doing proper ordering for later
users (like ipu, etc). I'd like to fix this in the best way possibly
so that fw_devlink can do the best ordering it can.
I just learned from Michael that there are other cases where fw_devlink
follows of_graph links from capture devices to connector nodes that have
a compatible, such as "composite-video-connector" or "svideo-connector",
but no associated driver. These are just parsed to determine the
connector type in drivers/media/v4l2-core/v4l2-fwnode.c. Would you
suggest to create platform devices for these as well?
remote-endpoints always point to each other. So they create dependency
cycles and fw_devlink stops enforcing ordering between them (because
it's a cycle and it has no way to know which one depends on the other
for probing). So for this specific case, you should get away without
writing platform drivers.
But in general, if you have a compatible string, there really aren't
that many good reasons to not use a proper driver for it.
-Saravana
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Philipp Zabel <p.zabel@pengutronix.de> Date: 2021-09-22 16:33:24
On Wed, 2021-09-22 at 08:38 -0700, Saravana Kannan wrote:
[...]
Yes, this node is handled by platform code in arch/arm/mach-imx/src.c as
Yes, I saw the node is handled in that file. I know it's not currently
a platform driver. I wasn't asking about that. I'm asking if a struct
device is created for the node or not. Creation of the struct device
is done automatically by the OF code and I don't see any code that
prevents the creation of the device.
Can you please run this on your device and that should answer my question"
# find /sys/devices/ -name *reset-controller*
I'm asking this question because there is a cleaner way to fix this
instead of what you are doing if there is actually a struct device
created for the node.
Oh, I wasn't aware. Yes,
/sys/devices/platform/soc/2000000.bus/20d8000.reset-controller
does exist.
quoted
the reset controller register also contains bits to bring up the CPUs.
Ok, I initially assumed that's why you were doing this, but I didn't
see this &src node (the reset controller) being referenced anywhere
else in DT. So how does that work?
There are a few functions exported from src.c (imx_enable_cpu,
imx_set_cpu_arg/jump) that are used in hotplug.c and platsmp.c.
quoted
quoted
Based on the DT, it looks like a device would automatically
get created for this node. So it looks like it's more of an issue with
the device not being probed by a real driver? Why can't you convert
the reset controller driver to a proper platform driver instead of
this patch? I think that's the right fix.
In this case, I could indeed add a platform driver for the reset
controller part. I'm not yet sure that this is always the right thing.
Can you dig into that more please? Instead of just deleting the fwnode
links? There are also ways to make sure you don't block early users
(before driver core is up) while still doing proper ordering for later
users (like ipu, etc). I'd like to fix this in the best way possibly
so that fw_devlink can do the best ordering it can.
I'll send a patch adding the platform driver, but could you elaborate on
what could be done with the auto-generated reset-controller device
instead?
[...]
remote-endpoints always point to each other. So they create dependency
cycles and fw_devlink stops enforcing ordering between them (because
it's a cycle and it has no way to know which one depends on the other
for probing). So for this specific case, you should get away without
writing platform drivers.
Right, thank you, I assume there must have been a missing backlink then.
regards
Philipp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Sep 22, 2021 at 9:31 AM Philipp Zabel [off-list ref] wrote:
On Wed, 2021-09-22 at 08:38 -0700, Saravana Kannan wrote:
[...]
quoted
Yes, this node is handled by platform code in arch/arm/mach-imx/src.c as
Yes, I saw the node is handled in that file. I know it's not currently
a platform driver. I wasn't asking about that. I'm asking if a struct
device is created for the node or not. Creation of the struct device
is done automatically by the OF code and I don't see any code that
prevents the creation of the device.
Can you please run this on your device and that should answer my question"
# find /sys/devices/ -name *reset-controller*
I'm asking this question because there is a cleaner way to fix this
instead of what you are doing if there is actually a struct device
created for the node.
Oh, I wasn't aware. Yes,
/sys/devices/platform/soc/2000000.bus/20d8000.reset-controller
does exist.
quoted
quoted
the reset controller register also contains bits to bring up the CPUs.
Ok, I initially assumed that's why you were doing this, but I didn't
see this &src node (the reset controller) being referenced anywhere
else in DT. So how does that work?
There are a few functions exported from src.c (imx_enable_cpu,
imx_set_cpu_arg/jump) that are used in hotplug.c and platsmp.c.
quoted
quoted
quoted
Based on the DT, it looks like a device would automatically
get created for this node. So it looks like it's more of an issue with
the device not being probed by a real driver? Why can't you convert
the reset controller driver to a proper platform driver instead of
this patch? I think that's the right fix.
In this case, I could indeed add a platform driver for the reset
controller part. I'm not yet sure that this is always the right thing.
Can you dig into that more please? Instead of just deleting the fwnode
links? There are also ways to make sure you don't block early users
(before driver core is up) while still doing proper ordering for later
users (like ipu, etc). I'd like to fix this in the best way possibly
so that fw_devlink can do the best ordering it can.
I'll send a patch adding the platform driver, but could you elaborate on
what could be done with the auto-generated reset-controller device
instead?
Your platform driver would basically match with this auto generated
platform device. You would just change arch/arm/mach-imx/src.c to add
platform_driver and the probe() would pretty much just do something
like:
... probe(...)
{
WARN_ON(!src_base);
imx_reset_controller.of_node = pdev->dev.of_node;
return reset_controller_register(&imx_reset_controller);
}
I don't think you need the IS_ENABLED() check since
reset_controller_register() is already stubbed out based on that.
You'll also leave the rest of the code in imx_src_int() since you'll
need those for your CPU reset to work.
Another option you could do instead of writing a platform driver is
something like this:
https://lore.kernel.org/lkml/20210205222644.2357303-8-saravanak@google.com/
But the benefit of writing a platform driver is that you'll also get
proper suspend/resume ordering (it might not matter in your case)
between the reset controller and it's non-CPU consumers.
I'm okay with you picking either option if you don't have any
suspend/resume ordering requirements between src vs its consumers, but
I think one of these options is better than the current patch that
just deletes the fwnode links.
-Saravana
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Michael Tretter <m.tretter@pengutronix.de> Date: 2021-09-23 08:19:25
On Wed, 22 Sep 2021 18:31:17 +0200, Philipp Zabel wrote:
On Wed, 2021-09-22 at 08:38 -0700, Saravana Kannan wrote:
[...]
quoted
remote-endpoints always point to each other. So they create dependency
cycles and fw_devlink stops enforcing ordering between them (because
it's a cycle and it has no way to know which one depends on the other
for probing). So for this specific case, you should get away without
writing platform drivers.
Right, thank you, I assume there must have been a missing backlink then.
I looked into this and found that remote-endpoints are actually working as
expected. The issue that I was observing was also caused by the missing reset
controller driver.
Thanks,
Michael
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel