Hello,
Linux 5.13-rc1 again has fw_devlink=on enabled by default. I've found that this
breaks probing display pipeline and HDMI output on sunxi boards, because of
fwnode_link between hdmi and hdmi-phy nodes.
HDMI device probe keeps being avoided with these repeated messages in dmesg:
platform 1ee0000.hdmi: probe deferral - supplier 1ef0000.hdmi-phy not ready
Both nodes have their own compatible, but are implemented by a single
struct device.
This looks like a kind of situation that's expected to break fw_devlink
expectations by my reading of the the e-mails about trying the fw_devlink=on
during 5.12 cycle.
Is this supposed to be solved by implementing the PHY node as it's own
device or by breaking the fwnode_link between the hdmi phy and hdmi nodes?
Seems like second solution would be quicker now that rc1 is out.
Where would be a good place to break that fwnode_link in code?
Or can the fw_devlink code be made aware of this situation, where two fwnodes
are implemented by the same Linux device, somehow?
Relevant references:
https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c#L600
https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c#L212
https://elixir.bootlin.com/linux/v5.13-rc1/source/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi#L1176
kind regards,
o.
On Sun, May 16, 2021 at 11:32:47PM -0700, Saravana Kannan wrote:
> On Sun, May 16, 2021 at 10:05 AM Ondřej Jirman <[email protected]> wrote:
> >
> > Hello,
> >
> > Linux 5.13-rc1 again has fw_devlink=on enabled by default. I've found that this
> > breaks probing display pipeline and HDMI output on sunxi boards, because of
> > fwnode_link between hdmi and hdmi-phy nodes.
> >
> > HDMI device probe keeps being avoided with these repeated messages in dmesg:
> >
> > platform 1ee0000.hdmi: probe deferral - supplier 1ef0000.hdmi-phy not ready
> >
> > Both nodes have their own compatible, but are implemented by a single
> > struct device.
> >
> > This looks like a kind of situation that's expected to break fw_devlink
> > expectations by my reading of the the e-mails about trying the fw_devlink=on
> > during 5.12 cycle.
> >
> > Is this supposed to be solved by implementing the PHY node as it's own
> > device or by breaking the fwnode_link between the hdmi phy and hdmi nodes?
> > Seems like second solution would be quicker now that rc1 is out.
>
> Seems like sun8i_hdmi_phy_probe() already does 95% of the work to make
> the PHY a separate driver. Why not just finish it up by really making
> it a separate driver? I'd really prefer doing that because this seems
> unnecessarily messed up. The phy will have a struct device created for
> it already. You are just not probing it.
Currently it's all just a glue code for dw-hdmi, which is not using a phy
framework and handles both the controller and phy parts. dw-hdmi needs passing
platform data around
(https://elixir.bootlin.com/linux/latest/source/include/drm/bridge/dw_hdmi.h#L115)
to get a specific set of phy glue callbacks hooked into platform data of dw-hdmi
prior to calling dw_hdmi_probe.
Looking at other users of dw_hdmi_probe this is the only one that has this
unfortunate issue due to using phys binding internally as a part of one device.
Just making it a platform driver will also change the probe order of phy and the
controller, which I've heard from Jernej needs to have the current order of
(controller and then phy) perserved, for some reason, and will make things
still a bit more convoluted.
So this looks like needs quite a bit of thought.
regards,
o.
> Thanks,
> Saravana
>
> >
> > Where would be a good place to break that fwnode_link in code?
> >
> > Or can the fw_devlink code be made aware of this situation, where two fwnodes
> > are implemented by the same Linux device, somehow?
> >
> > Relevant references:
> >
> > https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c#L600
> > https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c#L212
> > https://elixir.bootlin.com/linux/v5.13-rc1/source/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi#L1176
> >
> > kind regards,
> > o.
On Sun, May 16, 2021 at 10:05 AM Ondřej Jirman <[email protected]> wrote:
>
> Hello,
>
> Linux 5.13-rc1 again has fw_devlink=on enabled by default. I've found that this
> breaks probing display pipeline and HDMI output on sunxi boards, because of
> fwnode_link between hdmi and hdmi-phy nodes.
>
> HDMI device probe keeps being avoided with these repeated messages in dmesg:
>
> platform 1ee0000.hdmi: probe deferral - supplier 1ef0000.hdmi-phy not ready
>
> Both nodes have their own compatible, but are implemented by a single
> struct device.
>
> This looks like a kind of situation that's expected to break fw_devlink
> expectations by my reading of the the e-mails about trying the fw_devlink=on
> during 5.12 cycle.
>
> Is this supposed to be solved by implementing the PHY node as it's own
> device or by breaking the fwnode_link between the hdmi phy and hdmi nodes?
> Seems like second solution would be quicker now that rc1 is out.
Seems like sun8i_hdmi_phy_probe() already does 95% of the work to make
the PHY a separate driver. Why not just finish it up by really making
it a separate driver? I'd really prefer doing that because this seems
unnecessarily messed up. The phy will have a struct device created for
it already. You are just not probing it.
Thanks,
Saravana
>
> Where would be a good place to break that fwnode_link in code?
>
> Or can the fw_devlink code be made aware of this situation, where two fwnodes
> are implemented by the same Linux device, somehow?
>
> Relevant references:
>
> https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c#L600
> https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c#L212
> https://elixir.bootlin.com/linux/v5.13-rc1/source/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi#L1176
>
> kind regards,
> o.
On Mon, May 17, 2021 at 10:29:57AM +0200, Ondřej Jirman wrote:
> On Sun, May 16, 2021 at 11:32:47PM -0700, Saravana Kannan wrote:
> > On Sun, May 16, 2021 at 10:05 AM Ondřej Jirman <[email protected]> wrote:
> > >
> > > Hello,
> > >
> > > Linux 5.13-rc1 again has fw_devlink=on enabled by default. I've found that this
> > > breaks probing display pipeline and HDMI output on sunxi boards, because of
> > > fwnode_link between hdmi and hdmi-phy nodes.
> > >
> > > HDMI device probe keeps being avoided with these repeated messages in dmesg:
> > >
> > > platform 1ee0000.hdmi: probe deferral - supplier 1ef0000.hdmi-phy not ready
> > >
> > > Both nodes have their own compatible, but are implemented by a single
> > > struct device.
> > >
> > > This looks like a kind of situation that's expected to break fw_devlink
> > > expectations by my reading of the the e-mails about trying the fw_devlink=on
> > > during 5.12 cycle.
> > >
> > > Is this supposed to be solved by implementing the PHY node as it's own
> > > device or by breaking the fwnode_link between the hdmi phy and hdmi nodes?
> > > Seems like second solution would be quicker now that rc1 is out.
> >
> > Seems like sun8i_hdmi_phy_probe() already does 95% of the work to make
> > the PHY a separate driver. Why not just finish it up by really making
> > it a separate driver? I'd really prefer doing that because this seems
> > unnecessarily messed up. The phy will have a struct device created for
> > it already. You are just not probing it.
>
> Currently it's all just a glue code for dw-hdmi, which is not using a phy
> framework and handles both the controller and phy parts. dw-hdmi needs passing
> platform data around
> (https://elixir.bootlin.com/linux/latest/source/include/drm/bridge/dw_hdmi.h#L115)
> to get a specific set of phy glue callbacks hooked into platform data of dw-hdmi
> prior to calling dw_hdmi_probe.
>
> Looking at other users of dw_hdmi_probe this is the only one that has this
> unfortunate issue due to using phys binding internally as a part of one device.
>
> Just making it a platform driver will also change the probe order of phy and the
> controller, which I've heard from Jernej needs to have the current order of
> (controller and then phy) perserved, for some reason, and will make things
> still a bit more convoluted.
>
> So this looks like needs quite a bit of thought.
Yep, and even if it was simple (which it really isn't), it wouldn't be
-rc* material.
Maxime
On Mon, May 17, 2021 at 1:29 AM Ondřej Jirman <[email protected]> wrote:
>
> On Sun, May 16, 2021 at 11:32:47PM -0700, Saravana Kannan wrote:
> > On Sun, May 16, 2021 at 10:05 AM Ondřej Jirman <[email protected]> wrote:
> > >
> > > Hello,
> > >
> > > Linux 5.13-rc1 again has fw_devlink=on enabled by default. I've found that this
> > > breaks probing display pipeline and HDMI output on sunxi boards, because of
> > > fwnode_link between hdmi and hdmi-phy nodes.
> > >
> > > HDMI device probe keeps being avoided with these repeated messages in dmesg:
> > >
> > > platform 1ee0000.hdmi: probe deferral - supplier 1ef0000.hdmi-phy not ready
> > >
> > > Both nodes have their own compatible, but are implemented by a single
> > > struct device.
> > >
> > > This looks like a kind of situation that's expected to break fw_devlink
> > > expectations by my reading of the the e-mails about trying the fw_devlink=on
> > > during 5.12 cycle.
> > >
> > > Is this supposed to be solved by implementing the PHY node as it's own
> > > device or by breaking the fwnode_link between the hdmi phy and hdmi nodes?
> > > Seems like second solution would be quicker now that rc1 is out.
> >
> > Seems like sun8i_hdmi_phy_probe() already does 95% of the work to make
> > the PHY a separate driver. Why not just finish it up by really making
> > it a separate driver? I'd really prefer doing that because this seems
> > unnecessarily messed up. The phy will have a struct device created for
> > it already. You are just not probing it.
>
> Currently it's all just a glue code for dw-hdmi, which is not using a phy
> framework and handles both the controller and phy parts. dw-hdmi needs passing
> platform data around
> (https://elixir.bootlin.com/linux/latest/source/include/drm/bridge/dw_hdmi.h#L115)
> to get a specific set of phy glue callbacks hooked into platform data of dw-hdmi
> prior to calling dw_hdmi_probe.
>
> Looking at other users of dw_hdmi_probe this is the only one that has this
> unfortunate issue due to using phys binding internally as a part of one device.
>
> Just making it a platform driver will also change the probe order of phy and the
> controller, which I've heard from Jernej needs to have the current order of
> (controller and then phy) perserved, for some reason, and will make things
> still a bit more convoluted.
>
> So this looks like needs quite a bit of thought.
Nothing in sun8i_hdmi_phy_probe() depends on anything from
sun8i_dw_hdmi.c other than getting a struct device pointer to use with
dev_err and some devm_* APIs. So it seems pretty straightforward to
fix this so that you don't have one struct device trying to represent
two distinct hardware blocks. What am I missing?
Anyway, I took a swing at fixing this while preserving the ordering of
the important bits. The changes are fairly trivial/straightforward and
not meant to be final code, but can you test this out please?
-Saravana
Hello Saravana,
On Wed, May 19, 2021 at 02:29:48PM -0700, Saravana Kannan wrote:
>
> Nothing in sun8i_hdmi_phy_probe() depends on anything from
> sun8i_dw_hdmi.c other than getting a struct device pointer to use with
> dev_err and some devm_* APIs. So it seems pretty straightforward to
> fix this so that you don't have one struct device trying to represent
> two distinct hardware blocks. What am I missing?
>
> Anyway, I took a swing at fixing this while preserving the ordering of
> the important bits. The changes are fairly trivial/straightforward and
> not meant to be final code, but can you test this out please?
the patch seems to work, after fixing a few compilation issues. See
bellow. Thanks!
I think the probe order is reversed, but HDMI works in my case, so
I guess that doesn't matter in the end. Or at least it didn't in the
short test I made.
kind regards,
o.
From 0eac644368711f52fffa4246aefc546591cef090 Mon Sep 17 00:00:00 2001
From: Ondrej Jirman <[email protected]>
Date: Wed, 19 May 2021 23:44:45 +0200
Subject: [PATCH] fix compilation issues
---
drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
index c44ed22d8aef0..947b4231f6449 100644
--- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
+++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
@@ -5,6 +5,7 @@
#include <linux/delay.h>
#include <linux/of_address.h>
+#include <linux/of_platform.h>
#include "sun8i_dw_hdmi.h"
@@ -748,7 +749,7 @@ int sun8i_hdmi_phy_probe(struct platform_device *pdev)
return ret;
}
-void sun8i_hdmi_phy_remove(struct platform_device *pdev)
+int sun8i_hdmi_phy_remove(struct platform_device *pdev)
{
struct sun8i_hdmi_phy *phy = platform_get_drvdata(pdev);
--
2.31.1
[ 0.307254] platform 1c28000.serial: probe deferral - supplier 1c20800.pinctrl not ready
[ 0.307340] platform 1c28400.serial: probe deferral - supplier 1c20800.pinctrl not ready
[ 0.307379] platform 1c28c00.serial: probe deferral - supplier 1c20800.pinctrl not ready
[ 0.309235] fwnode_links_purge_suppliers: display-engine
[ 0.310176] platform 1c0c000.lcd-controller: probe deferral - wait for supplier mixer@200000
[ 0.310228] platform 1c0d000.lcd-controller: probe deferral - wait for supplier mixer@200000
[ 0.311902] platform 1ca0000.dsi: probe deferral - wait for supplier pmic@3a3
[ 0.312287] platform 1ee0000.hdmi: probe deferral - wait for supplier pmic@3a3
[ 0.313076] fwnode_links_purge_suppliers: hdmi-phy@1ef0000 <------ phy probed
[ 0.316766] platform 1c21800.lradc: probe deferral - wait for supplier pmic@3a3
[ 0.317258] platform vibrator: probe deferral - wait for supplier pmic@3a3
[ 0.318703] sun6i-rtc 1f00000.rtc: registered as rtc0
[ 0.318748] sun6i-rtc 1f00000.rtc: setting system clock to 2021-05-19T21:40:59 UTC (1621460459)
[ 0.318917] sun6i-rtc 1f00000.rtc: RTC enabled
[ 0.318938] fwnode_links_purge_suppliers: rtc@1f00000
[ 0.318960] fwnode_links_purge_consumers: reboot-mode@4
[ 0.319185] i2c /dev entries driver
....
[ 0.453301] fwnode_links_purge_consumers: port
[ 0.453323] fwnode_links_purge_consumers: endpoint
[ 0.453346] fwnode_links_purge_consumers: panel@0
[ 0.491868] sun4i-drm display-engine: bound 1100000.mixer (ops 0xffffffc010a44230)
[ 0.501830] sun4i-drm display-engine: bound 1200000.mixer (ops 0xffffffc010a44230)
[ 0.502915] sun4i-drm display-engine: No panel or bridge found... RGB output disabled
[ 0.502956] sun4i-drm display-engine: bound 1c0c000.lcd-controller (ops 0xffffffc010a415a0)
[ 0.503466] sun4i-drm display-engine: bound 1c0d000.lcd-controller (ops 0xffffffc010a415a0)
[ 0.503545] sun4i-drm display-engine: bound 1ca0000.dsi (ops 0xffffffc010a431c8)
[ 0.505330] sun8i-dw-hdmi 1ee0000.hdmi: Detected HDMI TX controller v1.32a with HDCP (sun8i_dw_hdmi_phy)
[ 0.507566] sun8i-dw-hdmi 1ee0000.hdmi: registered DesignWare HDMI I2C bus driver
[ 0.510717] sun4i-drm display-engine: bound 1ee0000.hdmi (ops 0xffffffc010a43568)
[ 0.514674] [drm] Initialized sun4i-drm 1.0.0 20150629 for display-engine on minor 0
[ 0.514932] sun4i-drm display-engine: [drm] Cannot find any crtc or sizes
[ 0.515160] fwnode_links_purge_suppliers: hdmi@1ee0000 <------ hdmi probed
[ 0.515195] fwnode_links_purge_consumers: ports
[ 0.515217] fwnode_links_purge_consumers: port@0
[ 0.515240] fwnode_links_purge_consumers: endpoint
[ 0.515264] fwnode_links_purge_consumers: port@1
[ 0.515343] fwnode_links_purge_consumers: endpoint
[ 0.517089] input: 1c21800.lradc as /devices/platform/soc/1c21800.lradc/input/input1
[ 0.517358] fwnode_links_purge_suppliers: lradc@1c21800
[ 0.517384] fwnode_links_purge_consumers: button-200
[ 0.517407] fwnode_links_purge_consumers: button-400
[ 0.518414] input: gpio-vibrator as /devices/platform/vibrator/input/input2
[ 0.518582] fwnode_links_purge_suppliers: vibrator
> -Saravana
On Wed, May 19, 2021 at 3:00 PM Ondřej Jirman <[email protected]> wrote:
>
> Hello Saravana,
>
> On Wed, May 19, 2021 at 02:29:48PM -0700, Saravana Kannan wrote:
> >
> > Nothing in sun8i_hdmi_phy_probe() depends on anything from
> > sun8i_dw_hdmi.c other than getting a struct device pointer to use with
> > dev_err and some devm_* APIs. So it seems pretty straightforward to
> > fix this so that you don't have one struct device trying to represent
> > two distinct hardware blocks. What am I missing?
> >
> > Anyway, I took a swing at fixing this while preserving the ordering of
> > the important bits. The changes are fairly trivial/straightforward and
> > not meant to be final code, but can you test this out please?
>
> the patch seems to work, after fixing a few compilation issues. See
> bellow. Thanks!
Thanks for the quick test! Do you want to fix it up and get it
upstreamed? I can do it, but I have other stuff I need to take care
of. Also, I just wanted to prove the change I was suggesting wasn't
complicated and fairly trivial.
> I think the probe order is reversed, but HDMI works in my case, so
> I guess that doesn't matter in the end. Or at least it didn't in the
> short test I made.
I don't think the "probe" order really changed. Firstly there was no
real phy "probe" before. However, I'm guessing the order that really
matters is the order between sun8i_hdmi_phy_init() and dw_hdmi_bind().
My patch still preserves that and that's why I think it works fine.
sun8i_hdmi_phy_probe() was just doing a bunch of "gets" on resources
so doing that a bit earlier seens pretty harmless.
-Saravana
>
> kind regards,
> o.
>
>
> From 0eac644368711f52fffa4246aefc546591cef090 Mon Sep 17 00:00:00 2001
> From: Ondrej Jirman <[email protected]>
> Date: Wed, 19 May 2021 23:44:45 +0200
> Subject: [PATCH] fix compilation issues
>
> ---
> drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
> index c44ed22d8aef0..947b4231f6449 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
> @@ -5,6 +5,7 @@
>
> #include <linux/delay.h>
> #include <linux/of_address.h>
> +#include <linux/of_platform.h>
>
> #include "sun8i_dw_hdmi.h"
>
> @@ -748,7 +749,7 @@ int sun8i_hdmi_phy_probe(struct platform_device *pdev)
> return ret;
> }
>
> -void sun8i_hdmi_phy_remove(struct platform_device *pdev)
> +int sun8i_hdmi_phy_remove(struct platform_device *pdev)
> {
> struct sun8i_hdmi_phy *phy = platform_get_drvdata(pdev);
>
> --
> 2.31.1
>
>
> [ 0.307254] platform 1c28000.serial: probe deferral - supplier 1c20800.pinctrl not ready
> [ 0.307340] platform 1c28400.serial: probe deferral - supplier 1c20800.pinctrl not ready
> [ 0.307379] platform 1c28c00.serial: probe deferral - supplier 1c20800.pinctrl not ready
> [ 0.309235] fwnode_links_purge_suppliers: display-engine
> [ 0.310176] platform 1c0c000.lcd-controller: probe deferral - wait for supplier mixer@200000
> [ 0.310228] platform 1c0d000.lcd-controller: probe deferral - wait for supplier mixer@200000
> [ 0.311902] platform 1ca0000.dsi: probe deferral - wait for supplier pmic@3a3
> [ 0.312287] platform 1ee0000.hdmi: probe deferral - wait for supplier pmic@3a3
> [ 0.313076] fwnode_links_purge_suppliers: hdmi-phy@1ef0000 <------ phy probed
> [ 0.316766] platform 1c21800.lradc: probe deferral - wait for supplier pmic@3a3
> [ 0.317258] platform vibrator: probe deferral - wait for supplier pmic@3a3
> [ 0.318703] sun6i-rtc 1f00000.rtc: registered as rtc0
> [ 0.318748] sun6i-rtc 1f00000.rtc: setting system clock to 2021-05-19T21:40:59 UTC (1621460459)
> [ 0.318917] sun6i-rtc 1f00000.rtc: RTC enabled
> [ 0.318938] fwnode_links_purge_suppliers: rtc@1f00000
> [ 0.318960] fwnode_links_purge_consumers: reboot-mode@4
> [ 0.319185] i2c /dev entries driver
>
> ....
>
> [ 0.453301] fwnode_links_purge_consumers: port
> [ 0.453323] fwnode_links_purge_consumers: endpoint
> [ 0.453346] fwnode_links_purge_consumers: panel@0
> [ 0.491868] sun4i-drm display-engine: bound 1100000.mixer (ops 0xffffffc010a44230)
> [ 0.501830] sun4i-drm display-engine: bound 1200000.mixer (ops 0xffffffc010a44230)
> [ 0.502915] sun4i-drm display-engine: No panel or bridge found... RGB output disabled
> [ 0.502956] sun4i-drm display-engine: bound 1c0c000.lcd-controller (ops 0xffffffc010a415a0)
> [ 0.503466] sun4i-drm display-engine: bound 1c0d000.lcd-controller (ops 0xffffffc010a415a0)
> [ 0.503545] sun4i-drm display-engine: bound 1ca0000.dsi (ops 0xffffffc010a431c8)
> [ 0.505330] sun8i-dw-hdmi 1ee0000.hdmi: Detected HDMI TX controller v1.32a with HDCP (sun8i_dw_hdmi_phy)
> [ 0.507566] sun8i-dw-hdmi 1ee0000.hdmi: registered DesignWare HDMI I2C bus driver
> [ 0.510717] sun4i-drm display-engine: bound 1ee0000.hdmi (ops 0xffffffc010a43568)
> [ 0.514674] [drm] Initialized sun4i-drm 1.0.0 20150629 for display-engine on minor 0
> [ 0.514932] sun4i-drm display-engine: [drm] Cannot find any crtc or sizes
> [ 0.515160] fwnode_links_purge_suppliers: hdmi@1ee0000 <------ hdmi probed
> [ 0.515195] fwnode_links_purge_consumers: ports
> [ 0.515217] fwnode_links_purge_consumers: port@0
> [ 0.515240] fwnode_links_purge_consumers: endpoint
> [ 0.515264] fwnode_links_purge_consumers: port@1
> [ 0.515343] fwnode_links_purge_consumers: endpoint
> [ 0.517089] input: 1c21800.lradc as /devices/platform/soc/1c21800.lradc/input/input1
> [ 0.517358] fwnode_links_purge_suppliers: lradc@1c21800
> [ 0.517384] fwnode_links_purge_consumers: button-200
> [ 0.517407] fwnode_links_purge_consumers: button-400
> [ 0.518414] input: gpio-vibrator as /devices/platform/vibrator/input/input2
> [ 0.518582] fwnode_links_purge_suppliers: vibrator
>
>
>
> > -Saravana