Re: fw_devlink=on and sunxi HDMI
From: Saravana Kannan
Date: Wed May 19 2021 - 17:30:30 EST
On Mon, May 17, 2021 at 1:29 AM Ondřej Jirman <megous@xxxxxxxxxx> 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 <megi@xxxxxx> 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
From f783941cdf4bd92c3da7a0c8441e6bd379087cc4 Mon Sep 17 00:00:00 2001
From: Saravana Kannan <saravanak@google.com>
Date: Wed, 19 May 2021 14:03:29 -0700
Subject: [PATCH] Try fixing HDMI probe.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 4 +--
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 3 +-
drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 41 ++++++++++++++++++++++----
3 files changed, 38 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
index bbdfd5e26ec8..e892a05e69e9 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
@@ -209,7 +209,7 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
goto err_disable_clk_tmds;
}
- ret = sun8i_hdmi_phy_probe(hdmi, phy_node);
+ ret = sun8i_hdmi_phy_get(hdmi, phy_node);
of_node_put(phy_node);
if (ret) {
dev_err(dev, "Couldn't get the HDMI PHY\n");
@@ -242,7 +242,6 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
cleanup_encoder:
drm_encoder_cleanup(encoder);
- sun8i_hdmi_phy_remove(hdmi);
err_disable_clk_tmds:
clk_disable_unprepare(hdmi->clk_tmds);
err_assert_ctrl_reset:
@@ -263,7 +262,6 @@ static void sun8i_dw_hdmi_unbind(struct device *dev, struct device *master,
struct sun8i_dw_hdmi *hdmi = dev_get_drvdata(dev);
dw_hdmi_unbind(hdmi->hdmi);
- sun8i_hdmi_phy_remove(hdmi);
clk_disable_unprepare(hdmi->clk_tmds);
reset_control_assert(hdmi->rst_ctrl);
gpiod_set_value(hdmi->ddc_en, 0);
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
index d4b55af0592f..8a8adef48be0 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
@@ -201,8 +201,7 @@ encoder_to_sun8i_dw_hdmi(struct drm_encoder *encoder)
return container_of(encoder, struct sun8i_dw_hdmi, encoder);
}
-int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, struct device_node *node);
-void sun8i_hdmi_phy_remove(struct sun8i_dw_hdmi *hdmi);
+int sun8i_hdmi_phy_get(struct sun8i_dw_hdmi *hdmi, struct device_node *node);
void sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy);
void sun8i_hdmi_phy_set_ops(struct sun8i_hdmi_phy *phy,
diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
index 9994edf67509..c44ed22d8aef 100644
--- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
+++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
@@ -597,10 +597,30 @@ static const struct of_device_id sun8i_hdmi_phy_of_table[] = {
{ /* sentinel */ }
};
-int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, struct device_node *node)
+int sun8i_hdmi_phy_get(struct sun8i_dw_hdmi *hdmi, struct device_node *node)
+{
+ struct platform_device *pdev = of_find_device_by_node(node);
+ struct sun8i_hdmi_phy *phy;
+
+ if (!pdev)
+ return -EPROBE_DEFER;
+
+ phy = platform_get_drvdata(pdev);
+ if (!phy)
+ return -EPROBE_DEFER;
+
+ hdmi->phy = phy;
+
+ put_device(&pdev->dev);
+
+ return 0;
+}
+
+int sun8i_hdmi_phy_probe(struct platform_device *pdev)
{
const struct of_device_id *match;
- struct device *dev = hdmi->dev;
+ struct device *dev = &pdev->dev;
+ struct device_node *node = dev->of_node;
struct sun8i_hdmi_phy *phy;
struct resource res;
void __iomem *regs;
@@ -704,7 +724,7 @@ int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, struct device_node *node)
clk_prepare_enable(phy->clk_phy);
}
- hdmi->phy = phy;
+ platform_set_drvdata(pdev, phy);
return 0;
@@ -728,9 +748,9 @@ int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, struct device_node *node)
return ret;
}
-void sun8i_hdmi_phy_remove(struct sun8i_dw_hdmi *hdmi)
+void sun8i_hdmi_phy_remove(struct platform_device *pdev)
{
- struct sun8i_hdmi_phy *phy = hdmi->phy;
+ struct sun8i_hdmi_phy *phy = platform_get_drvdata(pdev);
clk_disable_unprepare(phy->clk_mod);
clk_disable_unprepare(phy->clk_bus);
@@ -744,4 +764,15 @@ void sun8i_hdmi_phy_remove(struct sun8i_dw_hdmi *hdmi)
clk_put(phy->clk_pll1);
clk_put(phy->clk_mod);
clk_put(phy->clk_bus);
+ return 0;
}
+
+static struct platform_driver sun8i_hdmi_phy_driver = {
+ .probe = sun8i_hdmi_phy_probe,
+ .remove = sun8i_hdmi_phy_remove,
+ .driver = {
+ .name = "sun8i-hdmi-phy",
+ .of_match_table = sun8i_hdmi_phy_of_table,
+ },
+};
+module_platform_driver(sun8i_hdmi_phy_driver);
--
2.31.1.818.g46aad6cb9e-goog