[PATCH 00/14] improve Analogix DP AUX channel handling

[PATCH 00/14] improve Analogix DP AUX channel handling

[Lucas Stach]

Currently the AUX channel support in the Analogix DP driver is severely
limited as the AUX block of the bridge is only initialized when the video
link is to be enabled. This is okay for the purposes of link training,
but does not allow to detect displays by probing for EDID. This series
reworks the driver to allow AUX transactions before the video link is
active.

As this requires to rework some of the controller initialization and
also handling of both internal and external clocks, the series includes
quite a few changes to add better runtime PM handling.

Lucas Stach (14):
  drm/bridge: analogix_dp: remove unused platform power_on_end callback
  drm/rockchip: analogix_dp: add runtime PM handling
  drm/bridge: analogix_dp: register AUX bus after enabling runtime PM
  drm/bridge: analogix_dp: handle clock via runtime PM
  drm/bridge: analogix_dp: remove unused analogix_dp_remove
  drm/bridge: analogix_dp: remove clk handling from
    analogix_dp_set_bridge
  drm/bridge: analogix_dp: move platform and PHY power handling into
    runtime PM
  drm/bridge: analogix_dp: move basic controller init into runtime PM
  drm/bridge: analogix_dp: remove PLL lock check from
    analogix_dp_config_video
  drm/bridge: analogix_dp: move macro reset after link bandwidth setting
  drm/bridge: analogix_dp: don't wait for PLL lock too early
  drm/bridge: analogix_dp: simplify and correct PLL lock checks
  drm/bridge: analogix_dp: only read AUX status when an error occured
  drm/bridge: analogix_dp: handle AUX transfer timeouts

 .../drm/bridge/analogix/analogix_dp_core.c    | 196 ++++++++----------
 .../drm/bridge/analogix/analogix_dp_core.h    |   7 +-
 .../gpu/drm/bridge/analogix/analogix_dp_reg.c |  38 ++--
 .../gpu/drm/bridge/analogix/analogix_dp_reg.h |   9 +
 drivers/gpu/drm/exynos/exynos_dp.c            |   5 +-
 .../gpu/drm/rockchip/analogix_dp-rockchip.c   |  26 +--
 include/drm/bridge/analogix_dp.h              |   4 +-
 7 files changed, 120 insertions(+), 165 deletions(-)

-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 01/14] drm/bridge: analogix_dp: remove unused platform power_on_end callback

[Lucas Stach]

This isn't used, but gives the impression of the power on and power off
platform calls being non-symmetrical. Remove the unused callback and
rename the power_on_start to simplay power_on.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 7 ++-----
 drivers/gpu/drm/exynos/exynos_dp.c                 | 2 +-
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 4 ++--
 include/drm/bridge/analogix_dp.h                   | 3 +--
 4 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 98454f0af90e..b39721588980 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1264,8 +1264,8 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
 		goto out_dp_clk_pre;
 	}
 
-	if (dp->plat_data->power_on_start)
-		dp->plat_data->power_on_start(dp->plat_data);
+	if (dp->plat_data->power_on)
+		dp->plat_data->power_on(dp->plat_data);
 
 	phy_power_on(dp->phy);
 
@@ -1290,9 +1290,6 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
 		goto out_dp_init;
 	}
 
-	if (dp->plat_data->power_on_end)
-		dp->plat_data->power_on_end(dp->plat_data);
-
 	enable_irq(dp->irq);
 	return 0;
 
diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
index f48c4343f469..30c8750187ad 100644
--- a/drivers/gpu/drm/exynos/exynos_dp.c
+++ b/drivers/gpu/drm/exynos/exynos_dp.c
@@ -233,7 +233,7 @@ static int exynos_dp_probe(struct platform_device *pdev)
 	/* The remote port can be either a panel or a bridge */
 	dp->plat_data.panel = panel;
 	dp->plat_data.dev_type = EXYNOS_DP;
-	dp->plat_data.power_on_start = exynos_dp_poweron;
+	dp->plat_data.power_on = exynos_dp_poweron;
 	dp->plat_data.power_off = exynos_dp_poweroff;
 	dp->plat_data.attach = exynos_dp_bridge_attach;
 	dp->plat_data.get_modes = exynos_dp_get_modes;
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 7069a3d4d581..baeb41875a4b 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -92,7 +92,7 @@ static int rockchip_dp_pre_init(struct rockchip_dp_device *dp)
 	return 0;
 }
 
-static int rockchip_dp_poweron_start(struct analogix_dp_plat_data *plat_data)
+static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data)
 {
 	struct rockchip_dp_device *dp = pdata_encoder_to_dp(plat_data);
 	int ret;
@@ -397,7 +397,7 @@ static int rockchip_dp_probe(struct platform_device *pdev)
 	dp->data = dp_data;
 	dp->plat_data.panel = panel;
 	dp->plat_data.dev_type = dp->data->chip_type;
-	dp->plat_data.power_on_start = rockchip_dp_poweron_start;
+	dp->plat_data.power_on = rockchip_dp_poweron;
 	dp->plat_data.power_off = rockchip_dp_powerdown;
 	dp->plat_data.get_modes = rockchip_dp_get_modes;
 
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index b0dcc07334a1..8709b6a74c0f 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -29,8 +29,7 @@ struct analogix_dp_plat_data {
 	struct drm_connector *connector;
 	bool skip_connector;
 
-	int (*power_on_start)(struct analogix_dp_plat_data *);
-	int (*power_on_end)(struct analogix_dp_plat_data *);
+	int (*power_on)(struct analogix_dp_plat_data *);
 	int (*power_off)(struct analogix_dp_plat_data *);
 	int (*attach)(struct analogix_dp_plat_data *, struct drm_bridge *,
 		      struct drm_connector *);
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/14] drm/bridge: analogix_dp: remove unused platform power_on_end callback

[Robert Foss]

On Fri, May 3, 2024 at 5:12 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> This isn't used, but gives the impression of the power on and power off
> platform calls being non-symmetrical. Remove the unused callback and
> rename the power_on_start to simplay power_on.

s/simplay/simply

>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 7 ++-----
>  drivers/gpu/drm/exynos/exynos_dp.c                 | 2 +-
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 4 ++--
>  include/drm/bridge/analogix_dp.h                   | 3 +--
>  4 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 98454f0af90e..b39721588980 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1264,8 +1264,8 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
>                 goto out_dp_clk_pre;
>         }
>
> -       if (dp->plat_data->power_on_start)
> -               dp->plat_data->power_on_start(dp->plat_data);
> +       if (dp->plat_data->power_on)
> +               dp->plat_data->power_on(dp->plat_data);
>
>         phy_power_on(dp->phy);
>
> @@ -1290,9 +1290,6 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
>                 goto out_dp_init;
>         }
>
> -       if (dp->plat_data->power_on_end)
> -               dp->plat_data->power_on_end(dp->plat_data);
> -
>         enable_irq(dp->irq);
>         return 0;
>
> diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
> index f48c4343f469..30c8750187ad 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp.c
> @@ -233,7 +233,7 @@ static int exynos_dp_probe(struct platform_device *pdev)
>         /* The remote port can be either a panel or a bridge */
>         dp->plat_data.panel = panel;
>         dp->plat_data.dev_type = EXYNOS_DP;
> -       dp->plat_data.power_on_start = exynos_dp_poweron;
> +       dp->plat_data.power_on = exynos_dp_poweron;
>         dp->plat_data.power_off = exynos_dp_poweroff;
>         dp->plat_data.attach = exynos_dp_bridge_attach;
>         dp->plat_data.get_modes = exynos_dp_get_modes;
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 7069a3d4d581..baeb41875a4b 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -92,7 +92,7 @@ static int rockchip_dp_pre_init(struct rockchip_dp_device *dp)
>         return 0;
>  }
>
> -static int rockchip_dp_poweron_start(struct analogix_dp_plat_data *plat_data)
> +static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data)
>  {
>         struct rockchip_dp_device *dp = pdata_encoder_to_dp(plat_data);
>         int ret;
> @@ -397,7 +397,7 @@ static int rockchip_dp_probe(struct platform_device *pdev)
>         dp->data = dp_data;
>         dp->plat_data.panel = panel;
>         dp->plat_data.dev_type = dp->data->chip_type;
> -       dp->plat_data.power_on_start = rockchip_dp_poweron_start;
> +       dp->plat_data.power_on = rockchip_dp_poweron;
>         dp->plat_data.power_off = rockchip_dp_powerdown;
>         dp->plat_data.get_modes = rockchip_dp_get_modes;
>
> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> index b0dcc07334a1..8709b6a74c0f 100644
> --- a/include/drm/bridge/analogix_dp.h
> +++ b/include/drm/bridge/analogix_dp.h
> @@ -29,8 +29,7 @@ struct analogix_dp_plat_data {
>         struct drm_connector *connector;
>         bool skip_connector;
>
> -       int (*power_on_start)(struct analogix_dp_plat_data *);
> -       int (*power_on_end)(struct analogix_dp_plat_data *);
> +       int (*power_on)(struct analogix_dp_plat_data *);
>         int (*power_off)(struct analogix_dp_plat_data *);
>         int (*attach)(struct analogix_dp_plat_data *, struct drm_bridge *,
>                       struct drm_connector *);
> --
> 2.39.2
>

Reviewed-by: Robert Foss <rfoss@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 02/14] drm/rockchip: analogix_dp: add runtime PM handling

[Lucas Stach]

Hook up the runtime PM suspend/resume paths to make the rockchip
glue behave more like the exynos one. The same suspend/resume
functions are used for system sleep via the runtime PM force
suspend/resume.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index baeb41875a4b..8214265f1497 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -13,6 +13,7 @@
 #include <linux/of.h>
 #include <linux/of_graph.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
 #include <linux/clk.h>
@@ -430,7 +431,6 @@ static void rockchip_dp_remove(struct platform_device *pdev)
 	analogix_dp_remove(dp->adp);
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int rockchip_dp_suspend(struct device *dev)
 {
 	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
@@ -450,14 +450,9 @@ static int rockchip_dp_resume(struct device *dev)
 
 	return analogix_dp_resume(dp->adp);
 }
-#endif
 
-static const struct dev_pm_ops rockchip_dp_pm_ops = {
-#ifdef CONFIG_PM_SLEEP
-	.suspend_late = rockchip_dp_suspend,
-	.resume_early = rockchip_dp_resume,
-#endif
-};
+static DEFINE_RUNTIME_DEV_PM_OPS(rockchip_dp_pm_ops, rockchip_dp_suspend,
+		rockchip_dp_resume, NULL);
 
 static const struct rockchip_dp_chip_data rk3399_edp = {
 	.lcdsel_grf_reg = RK3399_GRF_SOC_CON20,
@@ -485,7 +480,7 @@ struct platform_driver rockchip_dp_driver = {
 	.remove_new = rockchip_dp_remove,
 	.driver = {
 		   .name = "rockchip-dp",
-		   .pm = &rockchip_dp_pm_ops,
+		   .pm = pm_ptr(&rockchip_dp_pm_ops),
 		   .of_match_table = rockchip_dp_dt_ids,
 	},
 };
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/14] drm/rockchip: analogix_dp: add runtime PM handling

[Robert Foss]

On Fri, May 3, 2024 at 5:12 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hook up the runtime PM suspend/resume paths to make the rockchip
> glue behave more like the exynos one. The same suspend/resume
> functions are used for system sleep via the runtime PM force
> suspend/resume.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index baeb41875a4b..8214265f1497 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -13,6 +13,7 @@
>  #include <linux/of.h>
>  #include <linux/of_graph.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
>  #include <linux/clk.h>
> @@ -430,7 +431,6 @@ static void rockchip_dp_remove(struct platform_device *pdev)
>         analogix_dp_remove(dp->adp);
>  }
>
> -#ifdef CONFIG_PM_SLEEP
>  static int rockchip_dp_suspend(struct device *dev)
>  {
>         struct rockchip_dp_device *dp = dev_get_drvdata(dev);
> @@ -450,14 +450,9 @@ static int rockchip_dp_resume(struct device *dev)
>
>         return analogix_dp_resume(dp->adp);
>  }
> -#endif
>
> -static const struct dev_pm_ops rockchip_dp_pm_ops = {
> -#ifdef CONFIG_PM_SLEEP
> -       .suspend_late = rockchip_dp_suspend,
> -       .resume_early = rockchip_dp_resume,
> -#endif
> -};
> +static DEFINE_RUNTIME_DEV_PM_OPS(rockchip_dp_pm_ops, rockchip_dp_suspend,
> +               rockchip_dp_resume, NULL);
>
>  static const struct rockchip_dp_chip_data rk3399_edp = {
>         .lcdsel_grf_reg = RK3399_GRF_SOC_CON20,
> @@ -485,7 +480,7 @@ struct platform_driver rockchip_dp_driver = {
>         .remove_new = rockchip_dp_remove,
>         .driver = {
>                    .name = "rockchip-dp",
> -                  .pm = &rockchip_dp_pm_ops,
> +                  .pm = pm_ptr(&rockchip_dp_pm_ops),
>                    .of_match_table = rockchip_dp_dt_ids,
>         },
>  };
> --
> 2.39.2
>

Reviewed-by: Robert Foss <rfoss@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/14] drm/rockchip: analogix_dp: add runtime PM handling

[Heiko Stübner]

Am Freitag, 3. Mai 2024, 17:11:17 CEST schrieb Lucas Stach:
> Hook up the runtime PM suspend/resume paths to make the rockchip
> glue behave more like the exynos one. The same suspend/resume
> functions are used for system sleep via the runtime PM force
> suspend/resume.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

A rk3399-gru Chromebook was able to suspend and wake up again
with working display both before and after.


Heiko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 03/14] drm/bridge: analogix_dp: register AUX bus after enabling runtime PM

[Lucas Stach]

AUX transactions require the controller to be in working state and
take a runtime PM reference. To avoid potential races beween the
first transactions on the bus and runtime PM being set up, move the
AUX registration behind the runtime PM setup.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 .../drm/bridge/analogix/analogix_dp_core.c    | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index b39721588980..0af2a70ae5bf 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1728,31 +1728,34 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
 	dp->drm_dev = drm_dev;
 	dp->encoder = dp->plat_data->encoder;
 
+	pm_runtime_use_autosuspend(dp->dev);
+	pm_runtime_set_autosuspend_delay(dp->dev, 100);
+	pm_runtime_enable(dp->dev);
+
 	dp->aux.name = "DP-AUX";
 	dp->aux.transfer = analogix_dpaux_transfer;
 	dp->aux.dev = dp->dev;
 	dp->aux.drm_dev = drm_dev;
 
 	ret = drm_dp_aux_register(&dp->aux);
-	if (ret)
-		return ret;
-
-	pm_runtime_use_autosuspend(dp->dev);
-	pm_runtime_set_autosuspend_delay(dp->dev, 100);
-	pm_runtime_enable(dp->dev);
+	if (ret) {
+		DRM_ERROR("failed to register AUX (%d)\n", ret);
+		goto err_disable_pm_runtime;
+	}
 
 	ret = analogix_dp_create_bridge(drm_dev, dp);
 	if (ret) {
 		DRM_ERROR("failed to create bridge (%d)\n", ret);
-		goto err_disable_pm_runtime;
+		goto err_unregister_aux;
 	}
 
 	return 0;
 
+err_unregister_aux:
+	drm_dp_aux_unregister(&dp->aux);
 err_disable_pm_runtime:
 	pm_runtime_dont_use_autosuspend(dp->dev);
 	pm_runtime_disable(dp->dev);
-	drm_dp_aux_unregister(&dp->aux);
 
 	return ret;
 }
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/14] drm/bridge: analogix_dp: register AUX bus after enabling runtime PM

[Robert Foss]

On Fri, May 3, 2024 at 5:12 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> AUX transactions require the controller to be in working state and
> take a runtime PM reference. To avoid potential races beween the
> first transactions on the bus and runtime PM being set up, move the
> AUX registration behind the runtime PM setup.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  .../drm/bridge/analogix/analogix_dp_core.c    | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index b39721588980..0af2a70ae5bf 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1728,31 +1728,34 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
>         dp->drm_dev = drm_dev;
>         dp->encoder = dp->plat_data->encoder;
>
> +       pm_runtime_use_autosuspend(dp->dev);
> +       pm_runtime_set_autosuspend_delay(dp->dev, 100);
> +       pm_runtime_enable(dp->dev);
> +
>         dp->aux.name = "DP-AUX";
>         dp->aux.transfer = analogix_dpaux_transfer;
>         dp->aux.dev = dp->dev;
>         dp->aux.drm_dev = drm_dev;
>
>         ret = drm_dp_aux_register(&dp->aux);
> -       if (ret)
> -               return ret;
> -
> -       pm_runtime_use_autosuspend(dp->dev);
> -       pm_runtime_set_autosuspend_delay(dp->dev, 100);
> -       pm_runtime_enable(dp->dev);
> +       if (ret) {
> +               DRM_ERROR("failed to register AUX (%d)\n", ret);
> +               goto err_disable_pm_runtime;
> +       }
>
>         ret = analogix_dp_create_bridge(drm_dev, dp);
>         if (ret) {
>                 DRM_ERROR("failed to create bridge (%d)\n", ret);
> -               goto err_disable_pm_runtime;
> +               goto err_unregister_aux;
>         }
>
>         return 0;
>
> +err_unregister_aux:
> +       drm_dp_aux_unregister(&dp->aux);
>  err_disable_pm_runtime:
>         pm_runtime_dont_use_autosuspend(dp->dev);
>         pm_runtime_disable(dp->dev);
> -       drm_dp_aux_unregister(&dp->aux);
>
>         return ret;
>  }
> --
> 2.39.2
>

Reviewed-by: Robert Foss <rfoss@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 04/14] drm/bridge: analogix_dp: handle clock via runtime PM

[Lucas Stach]

There is no reason to enable the controller clock in driver probe, as
there is no HW initialization done in this function. Instead rely on
either runtime PM to handle the controller clock or statically enable
it in the driver bind routine, after which real hardware access is
required to work.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 .../drm/bridge/analogix/analogix_dp_core.c    | 78 +++++++++++--------
 1 file changed, 45 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 0af2a70ae5bf..9e3308257586 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1658,8 +1658,6 @@ analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data)
 		return ERR_CAST(dp->clock);
 	}
 
-	clk_prepare_enable(dp->clock);
-
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
 	dp->reg_base = devm_ioremap_resource(&pdev->dev, res);
@@ -1721,6 +1719,29 @@ analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data)
 }
 EXPORT_SYMBOL_GPL(analogix_dp_probe);
 
+
+int analogix_dp_suspend(struct analogix_dp_device *dp)
+{
+	clk_disable_unprepare(dp->clock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(analogix_dp_suspend);
+
+int analogix_dp_resume(struct analogix_dp_device *dp)
+{
+	int ret;
+
+	ret = clk_prepare_enable(dp->clock);
+	if (ret < 0) {
+		DRM_ERROR("Failed to prepare_enable the clock clk [%d]\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(analogix_dp_resume);
+
 int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
 {
 	int ret;
@@ -1728,9 +1749,15 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
 	dp->drm_dev = drm_dev;
 	dp->encoder = dp->plat_data->encoder;
 
-	pm_runtime_use_autosuspend(dp->dev);
-	pm_runtime_set_autosuspend_delay(dp->dev, 100);
-	pm_runtime_enable(dp->dev);
+	if (IS_ENABLED(CONFIG_PM)) {
+		pm_runtime_use_autosuspend(dp->dev);
+		pm_runtime_set_autosuspend_delay(dp->dev, 100);
+		pm_runtime_enable(dp->dev);
+	} else {
+		ret = analogix_dp_resume(dp);
+		if (ret)
+			return ret;
+	}
 
 	dp->aux.name = "DP-AUX";
 	dp->aux.transfer = analogix_dpaux_transfer;
@@ -1754,8 +1781,12 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
 err_unregister_aux:
 	drm_dp_aux_unregister(&dp->aux);
 err_disable_pm_runtime:
-	pm_runtime_dont_use_autosuspend(dp->dev);
-	pm_runtime_disable(dp->dev);
+	if (IS_ENABLED(CONFIG_PM)) {
+		pm_runtime_dont_use_autosuspend(dp->dev);
+		pm_runtime_disable(dp->dev);
+	} else {
+		analogix_dp_suspend(dp);
+	}
 
 	return ret;
 }
@@ -1772,40 +1803,21 @@ void analogix_dp_unbind(struct analogix_dp_device *dp)
 	}
 
 	drm_dp_aux_unregister(&dp->aux);
-	pm_runtime_dont_use_autosuspend(dp->dev);
-	pm_runtime_disable(dp->dev);
+
+	if (IS_ENABLED(CONFIG_PM)) {
+		pm_runtime_dont_use_autosuspend(dp->dev);
+		pm_runtime_disable(dp->dev);
+	} else {
+		analogix_dp_suspend(dp);
+	}
 }
 EXPORT_SYMBOL_GPL(analogix_dp_unbind);
 
 void analogix_dp_remove(struct analogix_dp_device *dp)
 {
-	clk_disable_unprepare(dp->clock);
 }
 EXPORT_SYMBOL_GPL(analogix_dp_remove);
 
-#ifdef CONFIG_PM
-int analogix_dp_suspend(struct analogix_dp_device *dp)
-{
-	clk_disable_unprepare(dp->clock);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(analogix_dp_suspend);
-
-int analogix_dp_resume(struct analogix_dp_device *dp)
-{
-	int ret;
-
-	ret = clk_prepare_enable(dp->clock);
-	if (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the clock clk [%d]\n", ret);
-		return ret;
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(analogix_dp_resume);
-#endif
-
 int analogix_dp_start_crc(struct drm_connector *connector)
 {
 	struct analogix_dp_device *dp = to_dp(connector);
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/14] drm/bridge: analogix_dp: handle clock via runtime PM

[Robert Foss]

On Fri, May 3, 2024 at 5:12 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> There is no reason to enable the controller clock in driver probe, as
> there is no HW initialization done in this function. Instead rely on
> either runtime PM to handle the controller clock or statically enable
> it in the driver bind routine, after which real hardware access is
> required to work.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  .../drm/bridge/analogix/analogix_dp_core.c    | 78 +++++++++++--------
>  1 file changed, 45 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 0af2a70ae5bf..9e3308257586 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1658,8 +1658,6 @@ analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data)
>                 return ERR_CAST(dp->clock);
>         }
>
> -       clk_prepare_enable(dp->clock);
> -
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
>         dp->reg_base = devm_ioremap_resource(&pdev->dev, res);
> @@ -1721,6 +1719,29 @@ analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data)
>  }
>  EXPORT_SYMBOL_GPL(analogix_dp_probe);
>
> +
> +int analogix_dp_suspend(struct analogix_dp_device *dp)
> +{
> +       clk_disable_unprepare(dp->clock);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(analogix_dp_suspend);
> +
> +int analogix_dp_resume(struct analogix_dp_device *dp)
> +{
> +       int ret;
> +
> +       ret = clk_prepare_enable(dp->clock);
> +       if (ret < 0) {
> +               DRM_ERROR("Failed to prepare_enable the clock clk [%d]\n", ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(analogix_dp_resume);
> +
>  int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
>  {
>         int ret;
> @@ -1728,9 +1749,15 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
>         dp->drm_dev = drm_dev;
>         dp->encoder = dp->plat_data->encoder;
>
> -       pm_runtime_use_autosuspend(dp->dev);
> -       pm_runtime_set_autosuspend_delay(dp->dev, 100);
> -       pm_runtime_enable(dp->dev);
> +       if (IS_ENABLED(CONFIG_PM)) {
> +               pm_runtime_use_autosuspend(dp->dev);
> +               pm_runtime_set_autosuspend_delay(dp->dev, 100);
> +               pm_runtime_enable(dp->dev);
> +       } else {
> +               ret = analogix_dp_resume(dp);
> +               if (ret)
> +                       return ret;
> +       }
>
>         dp->aux.name = "DP-AUX";
>         dp->aux.transfer = analogix_dpaux_transfer;
> @@ -1754,8 +1781,12 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
>  err_unregister_aux:
>         drm_dp_aux_unregister(&dp->aux);
>  err_disable_pm_runtime:
> -       pm_runtime_dont_use_autosuspend(dp->dev);
> -       pm_runtime_disable(dp->dev);
> +       if (IS_ENABLED(CONFIG_PM)) {
> +               pm_runtime_dont_use_autosuspend(dp->dev);
> +               pm_runtime_disable(dp->dev);
> +       } else {
> +               analogix_dp_suspend(dp);
> +       }
>
>         return ret;
>  }
> @@ -1772,40 +1803,21 @@ void analogix_dp_unbind(struct analogix_dp_device *dp)
>         }
>
>         drm_dp_aux_unregister(&dp->aux);
> -       pm_runtime_dont_use_autosuspend(dp->dev);
> -       pm_runtime_disable(dp->dev);
> +
> +       if (IS_ENABLED(CONFIG_PM)) {
> +               pm_runtime_dont_use_autosuspend(dp->dev);
> +               pm_runtime_disable(dp->dev);
> +       } else {
> +               analogix_dp_suspend(dp);
> +       }
>  }
>  EXPORT_SYMBOL_GPL(analogix_dp_unbind);
>
>  void analogix_dp_remove(struct analogix_dp_device *dp)
>  {
> -       clk_disable_unprepare(dp->clock);
>  }
>  EXPORT_SYMBOL_GPL(analogix_dp_remove);
>
> -#ifdef CONFIG_PM
> -int analogix_dp_suspend(struct analogix_dp_device *dp)
> -{
> -       clk_disable_unprepare(dp->clock);
> -       return 0;
> -}
> -EXPORT_SYMBOL_GPL(analogix_dp_suspend);
> -
> -int analogix_dp_resume(struct analogix_dp_device *dp)
> -{
> -       int ret;
> -
> -       ret = clk_prepare_enable(dp->clock);
> -       if (ret < 0) {
> -               DRM_ERROR("Failed to prepare_enable the clock clk [%d]\n", ret);
> -               return ret;
> -       }
> -
> -       return 0;
> -}
> -EXPORT_SYMBOL_GPL(analogix_dp_resume);
> -#endif
> -
>  int analogix_dp_start_crc(struct drm_connector *connector)
>  {
>         struct analogix_dp_device *dp = to_dp(connector);
> --
> 2.39.2
>

Reviewed-by: Robert Foss <rfoss@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 05/14] drm/bridge: analogix_dp: remove unused analogix_dp_remove

[Lucas Stach]

Now that the clock is handled dynamically through
analogix_dp_resume/suspend and it isn't statically enabled in the
driver probe routine, there is no need for the remove function anymore.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 5 -----
 drivers/gpu/drm/exynos/exynos_dp.c                 | 3 ---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 9 +--------
 include/drm/bridge/analogix_dp.h                   | 1 -
 4 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 9e3308257586..9f1dfa6f2175 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1813,11 +1813,6 @@ void analogix_dp_unbind(struct analogix_dp_device *dp)
 }
 EXPORT_SYMBOL_GPL(analogix_dp_unbind);
 
-void analogix_dp_remove(struct analogix_dp_device *dp)
-{
-}
-EXPORT_SYMBOL_GPL(analogix_dp_remove);
-
 int analogix_dp_start_crc(struct drm_connector *connector)
 {
 	struct analogix_dp_device *dp = to_dp(connector);
diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
index 30c8750187ad..097f8c4617de 100644
--- a/drivers/gpu/drm/exynos/exynos_dp.c
+++ b/drivers/gpu/drm/exynos/exynos_dp.c
@@ -251,10 +251,7 @@ static int exynos_dp_probe(struct platform_device *pdev)
 
 static void exynos_dp_remove(struct platform_device *pdev)
 {
-	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
-
 	component_del(&pdev->dev, &exynos_dp_ops);
-	analogix_dp_remove(dp->adp);
 }
 
 static int exynos_dp_suspend(struct device *dev)
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 8214265f1497..362c7951ca4a 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -414,21 +414,14 @@ static int rockchip_dp_probe(struct platform_device *pdev)
 
 	ret = component_add(dev, &rockchip_dp_component_ops);
 	if (ret)
-		goto err_dp_remove;
+		return ret;
 
 	return 0;
-
-err_dp_remove:
-	analogix_dp_remove(dp->adp);
-	return ret;
 }
 
 static void rockchip_dp_remove(struct platform_device *pdev)
 {
-	struct rockchip_dp_device *dp = platform_get_drvdata(pdev);
-
 	component_del(&pdev->dev, &rockchip_dp_component_ops);
-	analogix_dp_remove(dp->adp);
 }
 
 static int rockchip_dp_suspend(struct device *dev)
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index 8709b6a74c0f..6002c5666031 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -44,7 +44,6 @@ struct analogix_dp_device *
 analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data);
 int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev);
 void analogix_dp_unbind(struct analogix_dp_device *dp);
-void analogix_dp_remove(struct analogix_dp_device *dp);
 
 int analogix_dp_start_crc(struct drm_connector *connector);
 int analogix_dp_stop_crc(struct drm_connector *connector);
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 05/14] drm/bridge: analogix_dp: remove unused analogix_dp_remove

[Robert Foss]

On Fri, May 3, 2024 at 5:13 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Now that the clock is handled dynamically through
> analogix_dp_resume/suspend and it isn't statically enabled in the
> driver probe routine, there is no need for the remove function anymore.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 5 -----
>  drivers/gpu/drm/exynos/exynos_dp.c                 | 3 ---
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 9 +--------
>  include/drm/bridge/analogix_dp.h                   | 1 -
>  4 files changed, 1 insertion(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 9e3308257586..9f1dfa6f2175 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1813,11 +1813,6 @@ void analogix_dp_unbind(struct analogix_dp_device *dp)
>  }
>  EXPORT_SYMBOL_GPL(analogix_dp_unbind);
>
> -void analogix_dp_remove(struct analogix_dp_device *dp)
> -{
> -}
> -EXPORT_SYMBOL_GPL(analogix_dp_remove);
> -
>  int analogix_dp_start_crc(struct drm_connector *connector)
>  {
>         struct analogix_dp_device *dp = to_dp(connector);
> diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
> index 30c8750187ad..097f8c4617de 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp.c
> @@ -251,10 +251,7 @@ static int exynos_dp_probe(struct platform_device *pdev)
>
>  static void exynos_dp_remove(struct platform_device *pdev)
>  {
> -       struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> -
>         component_del(&pdev->dev, &exynos_dp_ops);
> -       analogix_dp_remove(dp->adp);
>  }
>
>  static int exynos_dp_suspend(struct device *dev)
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 8214265f1497..362c7951ca4a 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -414,21 +414,14 @@ static int rockchip_dp_probe(struct platform_device *pdev)
>
>         ret = component_add(dev, &rockchip_dp_component_ops);
>         if (ret)
> -               goto err_dp_remove;
> +               return ret;
>
>         return 0;
> -
> -err_dp_remove:
> -       analogix_dp_remove(dp->adp);
> -       return ret;
>  }
>
>  static void rockchip_dp_remove(struct platform_device *pdev)
>  {
> -       struct rockchip_dp_device *dp = platform_get_drvdata(pdev);
> -
>         component_del(&pdev->dev, &rockchip_dp_component_ops);
> -       analogix_dp_remove(dp->adp);
>  }
>
>  static int rockchip_dp_suspend(struct device *dev)
> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> index 8709b6a74c0f..6002c5666031 100644
> --- a/include/drm/bridge/analogix_dp.h
> +++ b/include/drm/bridge/analogix_dp.h
> @@ -44,7 +44,6 @@ struct analogix_dp_device *
>  analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data);
>  int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev);
>  void analogix_dp_unbind(struct analogix_dp_device *dp);
> -void analogix_dp_remove(struct analogix_dp_device *dp);
>
>  int analogix_dp_start_crc(struct drm_connector *connector);
>  int analogix_dp_stop_crc(struct drm_connector *connector);
> --
> 2.39.2
>

Reviewed-by: Robert Foss <rfoss@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 06/14] drm/bridge: analogix_dp: remove clk handling from analogix_dp_set_bridge

[Lucas Stach]

The clock is already managed by runtime PM, which is properly invoked
from the analogix_dp_set_bridge function, so there is no need for an
additional reference.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 9f1dfa6f2175..311e1e67486d 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1258,12 +1258,6 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
 
 	pm_runtime_get_sync(dp->dev);
 
-	ret = clk_prepare_enable(dp->clock);
-	if (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the clock clk [%d]\n", ret);
-		goto out_dp_clk_pre;
-	}
-
 	if (dp->plat_data->power_on)
 		dp->plat_data->power_on(dp->plat_data);
 
@@ -1297,8 +1291,7 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
 	phy_power_off(dp->phy);
 	if (dp->plat_data->power_off)
 		dp->plat_data->power_off(dp->plat_data);
-	clk_disable_unprepare(dp->clock);
-out_dp_clk_pre:
+
 	pm_runtime_put_sync(dp->dev);
 
 	return ret;
@@ -1367,8 +1360,6 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
 	analogix_dp_set_analog_power_down(dp, POWER_ALL, 1);
 	phy_power_off(dp->phy);
 
-	clk_disable_unprepare(dp->clock);
-
 	pm_runtime_put_sync(dp->dev);
 
 	ret = analogix_dp_prepare_panel(dp, false, true);
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 06/14] drm/bridge: analogix_dp: remove clk handling from analogix_dp_set_bridge

[Robert Foss]

On Fri, May 3, 2024 at 5:12 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> The clock is already managed by runtime PM, which is properly invoked
> from the analogix_dp_set_bridge function, so there is no need for an
> additional reference.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 9f1dfa6f2175..311e1e67486d 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1258,12 +1258,6 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
>
>         pm_runtime_get_sync(dp->dev);
>
> -       ret = clk_prepare_enable(dp->clock);
> -       if (ret < 0) {
> -               DRM_ERROR("Failed to prepare_enable the clock clk [%d]\n", ret);
> -               goto out_dp_clk_pre;
> -       }
> -
>         if (dp->plat_data->power_on)
>                 dp->plat_data->power_on(dp->plat_data);
>
> @@ -1297,8 +1291,7 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
>         phy_power_off(dp->phy);
>         if (dp->plat_data->power_off)
>                 dp->plat_data->power_off(dp->plat_data);
> -       clk_disable_unprepare(dp->clock);
> -out_dp_clk_pre:
> +
>         pm_runtime_put_sync(dp->dev);
>
>         return ret;
> @@ -1367,8 +1360,6 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>         analogix_dp_set_analog_power_down(dp, POWER_ALL, 1);
>         phy_power_off(dp->phy);
>
> -       clk_disable_unprepare(dp->clock);
> -
>         pm_runtime_put_sync(dp->dev);
>
>         ret = analogix_dp_prepare_panel(dp, false, true);
> --
> 2.39.2
>

Reviewed-by: Robert Foss <rfoss@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 07/14] drm/bridge: analogix_dp: move platform and PHY power handling into runtime PM

[Lucas Stach]

Platform and PHY power isn't only required when the actual display data
stream is active, but may be required earlier to support AUX channel
transactions. Move them into the runtime PM calls, so they are properly
managed whenever various other parts of the driver need them to be active.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 .../drm/bridge/analogix/analogix_dp_core.c    | 23 ++++++++-----------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 311e1e67486d..3281c00a39cd 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1258,11 +1258,6 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
 
 	pm_runtime_get_sync(dp->dev);
 
-	if (dp->plat_data->power_on)
-		dp->plat_data->power_on(dp->plat_data);
-
-	phy_power_on(dp->phy);
-
 	ret = analogix_dp_init_dp(dp);
 	if (ret)
 		goto out_dp_init;
@@ -1288,10 +1283,6 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
 	return 0;
 
 out_dp_init:
-	phy_power_off(dp->phy);
-	if (dp->plat_data->power_off)
-		dp->plat_data->power_off(dp->plat_data);
-
 	pm_runtime_put_sync(dp->dev);
 
 	return ret;
@@ -1354,11 +1345,7 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
 
 	disable_irq(dp->irq);
 
-	if (dp->plat_data->power_off)
-		dp->plat_data->power_off(dp->plat_data);
-
 	analogix_dp_set_analog_power_down(dp, POWER_ALL, 1);
-	phy_power_off(dp->phy);
 
 	pm_runtime_put_sync(dp->dev);
 
@@ -1713,6 +1700,11 @@ EXPORT_SYMBOL_GPL(analogix_dp_probe);
 
 int analogix_dp_suspend(struct analogix_dp_device *dp)
 {
+	phy_power_off(dp->phy);
+
+	if (dp->plat_data->power_off)
+		dp->plat_data->power_off(dp->plat_data);
+
 	clk_disable_unprepare(dp->clock);
 
 	return 0;
@@ -1729,6 +1721,11 @@ int analogix_dp_resume(struct analogix_dp_device *dp)
 		return ret;
 	}
 
+	if (dp->plat_data->power_on)
+		dp->plat_data->power_on(dp->plat_data);
+
+	phy_power_on(dp->phy);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(analogix_dp_resume);
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/14] drm/bridge: analogix_dp: move platform and PHY power handling into runtime PM

[Robert Foss]

On Fri, May 3, 2024 at 5:12 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Platform and PHY power isn't only required when the actual display data
> stream is active, but may be required earlier to support AUX channel
> transactions. Move them into the runtime PM calls, so they are properly
> managed whenever various other parts of the driver need them to be active.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  .../drm/bridge/analogix/analogix_dp_core.c    | 23 ++++++++-----------
>  1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 311e1e67486d..3281c00a39cd 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1258,11 +1258,6 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
>
>         pm_runtime_get_sync(dp->dev);
>
> -       if (dp->plat_data->power_on)
> -               dp->plat_data->power_on(dp->plat_data);
> -
> -       phy_power_on(dp->phy);
> -
>         ret = analogix_dp_init_dp(dp);
>         if (ret)
>                 goto out_dp_init;
> @@ -1288,10 +1283,6 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
>         return 0;
>
>  out_dp_init:
> -       phy_power_off(dp->phy);
> -       if (dp->plat_data->power_off)
> -               dp->plat_data->power_off(dp->plat_data);
> -
>         pm_runtime_put_sync(dp->dev);
>
>         return ret;
> @@ -1354,11 +1345,7 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>
>         disable_irq(dp->irq);
>
> -       if (dp->plat_data->power_off)
> -               dp->plat_data->power_off(dp->plat_data);
> -
>         analogix_dp_set_analog_power_down(dp, POWER_ALL, 1);
> -       phy_power_off(dp->phy);
>
>         pm_runtime_put_sync(dp->dev);
>
> @@ -1713,6 +1700,11 @@ EXPORT_SYMBOL_GPL(analogix_dp_probe);
>
>  int analogix_dp_suspend(struct analogix_dp_device *dp)
>  {
> +       phy_power_off(dp->phy);
> +
> +       if (dp->plat_data->power_off)
> +               dp->plat_data->power_off(dp->plat_data);
> +
>         clk_disable_unprepare(dp->clock);
>
>         return 0;
> @@ -1729,6 +1721,11 @@ int analogix_dp_resume(struct analogix_dp_device *dp)
>                 return ret;
>         }
>
> +       if (dp->plat_data->power_on)
> +               dp->plat_data->power_on(dp->plat_data);
> +
> +       phy_power_on(dp->phy);
> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(analogix_dp_resume);
> --
> 2.39.2
>

Reviewed-by: Robert Foss <rfoss@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 08/14] drm/bridge: analogix_dp: move basic controller init into runtime PM

[Lucas Stach]

Make sure the controller is in a basic working state after runtime
resume. Keep the analog function enable in the mode set path as this
enables parts of the PHY that are only required to be powered when
there is a data stream being sent out.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 3281c00a39cd..fdb2c2a2b69a 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -41,10 +41,8 @@ struct bridge_init {
 	struct device_node *node;
 };
 
-static int analogix_dp_init_dp(struct analogix_dp_device *dp)
+static void analogix_dp_init_dp(struct analogix_dp_device *dp)
 {
-	int ret;
-
 	analogix_dp_reset(dp);
 
 	analogix_dp_swreset(dp);
@@ -56,13 +54,9 @@ static int analogix_dp_init_dp(struct analogix_dp_device *dp)
 	analogix_dp_enable_sw_function(dp);
 
 	analogix_dp_config_interrupt(dp);
-	ret = analogix_dp_init_analog_func(dp);
-	if (ret)
-		return ret;
 
 	analogix_dp_init_hpd(dp);
 	analogix_dp_init_aux(dp);
-	return 0;
 }
 
 static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
@@ -1258,9 +1252,9 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
 
 	pm_runtime_get_sync(dp->dev);
 
-	ret = analogix_dp_init_dp(dp);
+	ret = analogix_dp_init_analog_func(dp);
 	if (ret)
-		goto out_dp_init;
+		return ret;
 
 	/*
 	 * According to DP spec v1.3 chap 3.5.1.2 Link Training,
@@ -1726,6 +1720,8 @@ int analogix_dp_resume(struct analogix_dp_device *dp)
 
 	phy_power_on(dp->phy);
 
+	analogix_dp_init_dp(dp);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(analogix_dp_resume);
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/14] drm/bridge: analogix_dp: move basic controller init into runtime PM

[Robert Foss]

On Fri, May 3, 2024 at 5:12 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Make sure the controller is in a basic working state after runtime
> resume. Keep the analog function enable in the mode set path as this
> enables parts of the PHY that are only required to be powered when
> there is a data stream being sent out.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 3281c00a39cd..fdb2c2a2b69a 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -41,10 +41,8 @@ struct bridge_init {
>         struct device_node *node;
>  };
>
> -static int analogix_dp_init_dp(struct analogix_dp_device *dp)
> +static void analogix_dp_init_dp(struct analogix_dp_device *dp)
>  {
> -       int ret;
> -
>         analogix_dp_reset(dp);
>
>         analogix_dp_swreset(dp);
> @@ -56,13 +54,9 @@ static int analogix_dp_init_dp(struct analogix_dp_device *dp)
>         analogix_dp_enable_sw_function(dp);
>
>         analogix_dp_config_interrupt(dp);
> -       ret = analogix_dp_init_analog_func(dp);
> -       if (ret)
> -               return ret;
>
>         analogix_dp_init_hpd(dp);
>         analogix_dp_init_aux(dp);
> -       return 0;
>  }
>
>  static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
> @@ -1258,9 +1252,9 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
>
>         pm_runtime_get_sync(dp->dev);
>
> -       ret = analogix_dp_init_dp(dp);
> +       ret = analogix_dp_init_analog_func(dp);
>         if (ret)
> -               goto out_dp_init;
> +               return ret;
>
>         /*
>          * According to DP spec v1.3 chap 3.5.1.2 Link Training,
> @@ -1726,6 +1720,8 @@ int analogix_dp_resume(struct analogix_dp_device *dp)
>
>         phy_power_on(dp->phy);
>
> +       analogix_dp_init_dp(dp);
> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(analogix_dp_resume);
> --
> 2.39.2
>

Reviewed-by: Robert Foss <rfoss@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 09/14] drm/bridge: analogix_dp: remove PLL lock check from analogix_dp_config_video

[Lucas Stach]

This check is way too late in the DP enable flow. The PLL must be locked
much earlier, before any link training can happen. If the PLL is unlocked
at that point in time there is something seriously wrong in the enable flow.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index fdb2c2a2b69a..b4a47311cfe8 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -720,11 +720,6 @@ static int analogix_dp_config_video(struct analogix_dp_device *dp)
 
 	analogix_dp_set_video_color_format(dp);
 
-	if (analogix_dp_get_pll_lock_status(dp) == PLL_UNLOCKED) {
-		dev_err(dp->dev, "PLL is not locked yet.\n");
-		return -EINVAL;
-	}
-
 	for (;;) {
 		timeout_loop++;
 		if (analogix_dp_is_slave_video_stream_clock_on(dp) == 0)
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/14] drm/bridge: analogix_dp: remove PLL lock check from analogix_dp_config_video

[Robert Foss]

On Fri, May 3, 2024 at 5:13 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> This check is way too late in the DP enable flow. The PLL must be locked
> much earlier, before any link training can happen. If the PLL is unlocked
> at that point in time there is something seriously wrong in the enable flow.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index fdb2c2a2b69a..b4a47311cfe8 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -720,11 +720,6 @@ static int analogix_dp_config_video(struct analogix_dp_device *dp)
>
>         analogix_dp_set_video_color_format(dp);
>
> -       if (analogix_dp_get_pll_lock_status(dp) == PLL_UNLOCKED) {
> -               dev_err(dp->dev, "PLL is not locked yet.\n");
> -               return -EINVAL;
> -       }
> -
>         for (;;) {
>                 timeout_loop++;
>                 if (analogix_dp_is_slave_video_stream_clock_on(dp) == 0)
> --
> 2.39.2
>

Reviewed-by: Robet Foss <rfoss@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 10/14] drm/bridge: analogix_dp: move macro reset after link bandwidth setting

[Lucas Stach]

Setting the link bandwidth may change the PLL parameters, which will cause
the PLL to go out of lock, so make sure to apply the MACRO_RST, which
according to the comment is required to be pulsed after the PLL is locked.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 .../gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index b4a47311cfe8..736b2ed745e1 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -243,6 +243,11 @@ static int analogix_dp_link_start(struct analogix_dp_device *dp)
 
 	/* Set link rate and count as you want to establish*/
 	analogix_dp_set_link_bandwidth(dp, dp->link_train.link_rate);
+	/*
+	 * MACRO_RST must be applied after the PLL_LOCK to avoid
+	 * the DP inter pair skew issue for at least 10 us
+	 */
+	analogix_dp_reset_macro(dp);
 	analogix_dp_set_lane_count(dp, dp->link_train.lane_count);
 
 	/* Setup RX configuration */
@@ -565,12 +570,6 @@ static int analogix_dp_full_link_train(struct analogix_dp_device *dp,
 	int retval = 0;
 	bool training_finished = false;
 
-	/*
-	 * MACRO_RST must be applied after the PLL_LOCK to avoid
-	 * the DP inter pair skew issue for at least 10 us
-	 */
-	analogix_dp_reset_macro(dp);
-
 	/* Initialize by reading RX's DPCD */
 	analogix_dp_get_max_rx_bandwidth(dp, &dp->link_train.link_rate);
 	analogix_dp_get_max_rx_lane_count(dp, &dp->link_train.lane_count);
@@ -637,9 +636,12 @@ static int analogix_dp_fast_link_train(struct analogix_dp_device *dp)
 	u8 link_align, link_status[2];
 	enum pll_status status;
 
-	analogix_dp_reset_macro(dp);
-
 	analogix_dp_set_link_bandwidth(dp, dp->link_train.link_rate);
+	/*
+	 * MACRO_RST must be applied after the PLL_LOCK to avoid
+	 * the DP inter pair skew issue for at least 10 us
+	 */
+	analogix_dp_reset_macro(dp);
 	analogix_dp_set_lane_count(dp, dp->link_train.lane_count);
 	analogix_dp_set_lane_link_training(dp);
 
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 10/14] drm/bridge: analogix_dp: move macro reset after link bandwidth setting

[Robert Foss]

On Fri, May 3, 2024 at 5:13 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Setting the link bandwidth may change the PLL parameters, which will cause
> the PLL to go out of lock, so make sure to apply the MACRO_RST, which
> according to the comment is required to be pulsed after the PLL is locked.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  .../gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index b4a47311cfe8..736b2ed745e1 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -243,6 +243,11 @@ static int analogix_dp_link_start(struct analogix_dp_device *dp)
>
>         /* Set link rate and count as you want to establish*/
>         analogix_dp_set_link_bandwidth(dp, dp->link_train.link_rate);
> +       /*
> +        * MACRO_RST must be applied after the PLL_LOCK to avoid
> +        * the DP inter pair skew issue for at least 10 us
> +        */
> +       analogix_dp_reset_macro(dp);
>         analogix_dp_set_lane_count(dp, dp->link_train.lane_count);
>
>         /* Setup RX configuration */
> @@ -565,12 +570,6 @@ static int analogix_dp_full_link_train(struct analogix_dp_device *dp,
>         int retval = 0;
>         bool training_finished = false;
>
> -       /*
> -        * MACRO_RST must be applied after the PLL_LOCK to avoid
> -        * the DP inter pair skew issue for at least 10 us
> -        */
> -       analogix_dp_reset_macro(dp);
> -
>         /* Initialize by reading RX's DPCD */
>         analogix_dp_get_max_rx_bandwidth(dp, &dp->link_train.link_rate);
>         analogix_dp_get_max_rx_lane_count(dp, &dp->link_train.lane_count);
> @@ -637,9 +636,12 @@ static int analogix_dp_fast_link_train(struct analogix_dp_device *dp)
>         u8 link_align, link_status[2];
>         enum pll_status status;
>
> -       analogix_dp_reset_macro(dp);
> -
>         analogix_dp_set_link_bandwidth(dp, dp->link_train.link_rate);
> +       /*
> +        * MACRO_RST must be applied after the PLL_LOCK to avoid
> +        * the DP inter pair skew issue for at least 10 us
> +        */
> +       analogix_dp_reset_macro(dp);
>         analogix_dp_set_lane_count(dp, dp->link_train.lane_count);
>         analogix_dp_set_lane_link_training(dp);
>
> --
> 2.39.2
>

This patch does not apply on drm-misc/drm-misc-next as far as I can tell.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 10/14] drm/bridge: analogix_dp: move macro reset after link bandwidth setting

[Robert Foss]

On Tue, May 7, 2024 at 3:07 PM Robert Foss <rfoss@kernel.org> wrote:
>
> On Fri, May 3, 2024 at 5:13 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> >
> > Setting the link bandwidth may change the PLL parameters, which will cause
> > the PLL to go out of lock, so make sure to apply the MACRO_RST, which
> > according to the comment is required to be pulsed after the PLL is locked.
> >
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  .../gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > index b4a47311cfe8..736b2ed745e1 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > @@ -243,6 +243,11 @@ static int analogix_dp_link_start(struct analogix_dp_device *dp)
> >
> >         /* Set link rate and count as you want to establish*/
> >         analogix_dp_set_link_bandwidth(dp, dp->link_train.link_rate);
> > +       /*
> > +        * MACRO_RST must be applied after the PLL_LOCK to avoid
> > +        * the DP inter pair skew issue for at least 10 us
> > +        */
> > +       analogix_dp_reset_macro(dp);
> >         analogix_dp_set_lane_count(dp, dp->link_train.lane_count);
> >
> >         /* Setup RX configuration */
> > @@ -565,12 +570,6 @@ static int analogix_dp_full_link_train(struct analogix_dp_device *dp,
> >         int retval = 0;
> >         bool training_finished = false;
> >
> > -       /*
> > -        * MACRO_RST must be applied after the PLL_LOCK to avoid
> > -        * the DP inter pair skew issue for at least 10 us
> > -        */
> > -       analogix_dp_reset_macro(dp);
> > -
> >         /* Initialize by reading RX's DPCD */
> >         analogix_dp_get_max_rx_bandwidth(dp, &dp->link_train.link_rate);
> >         analogix_dp_get_max_rx_lane_count(dp, &dp->link_train.lane_count);
> > @@ -637,9 +636,12 @@ static int analogix_dp_fast_link_train(struct analogix_dp_device *dp)
> >         u8 link_align, link_status[2];
> >         enum pll_status status;
> >
> > -       analogix_dp_reset_macro(dp);
> > -
> >         analogix_dp_set_link_bandwidth(dp, dp->link_train.link_rate);
> > +       /*
> > +        * MACRO_RST must be applied after the PLL_LOCK to avoid
> > +        * the DP inter pair skew issue for at least 10 us
> > +        */
> > +       analogix_dp_reset_macro(dp);
> >         analogix_dp_set_lane_count(dp, dp->link_train.lane_count);
> >         analogix_dp_set_lane_link_training(dp);
> >
> > --
> > 2.39.2
> >
>
> This patch does not apply on drm-misc/drm-misc-next as far as I can tell.

Reviewed-by: Robert Foss <rfoss@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 11/14] drm/bridge: analogix_dp: don't wait for PLL lock too early

[Lucas Stach]

The PLL will be reconfigured later, which may cause it to go out of lock
anyways, so there is no point in waiting for the PLL to lock here. Instead
we can continue execution of the link setup, which will properly set the
PLL parameters and will wait for the PLL to lock at the appropriate times.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index d267cf05cbca..e9c643a8b6fc 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
@@ -356,7 +356,6 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
 int analogix_dp_init_analog_func(struct analogix_dp_device *dp)
 {
 	u32 reg;
-	int timeout_loop = 0;
 
 	analogix_dp_set_analog_power_down(dp, POWER_ALL, 0);
 
@@ -368,18 +367,7 @@ int analogix_dp_init_analog_func(struct analogix_dp_device *dp)
 	writel(reg, dp->reg_base + ANALOGIX_DP_DEBUG_CTL);
 
 	/* Power up PLL */
-	if (analogix_dp_get_pll_lock_status(dp) == PLL_UNLOCKED) {
-		analogix_dp_set_pll_power_down(dp, 0);
-
-		while (analogix_dp_get_pll_lock_status(dp) == PLL_UNLOCKED) {
-			timeout_loop++;
-			if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
-				dev_err(dp->dev, "failed to get pll lock status\n");
-				return -ETIMEDOUT;
-			}
-			usleep_range(10, 20);
-		}
-	}
+	analogix_dp_set_pll_power_down(dp, 0);
 
 	/* Enable Serdes FIFO function and Link symbol clock domain module */
 	reg = readl(dp->reg_base + ANALOGIX_DP_FUNC_EN_2);
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 11/14] drm/bridge: analogix_dp: don't wait for PLL lock too early

[Robert Foss]

On Fri, May 3, 2024 at 5:12 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> The PLL will be reconfigured later, which may cause it to go out of lock
> anyways, so there is no point in waiting for the PLL to lock here. Instead
> we can continue execution of the link setup, which will properly set the
> PLL parameters and will wait for the PLL to lock at the appropriate times.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index d267cf05cbca..e9c643a8b6fc 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -356,7 +356,6 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
>  int analogix_dp_init_analog_func(struct analogix_dp_device *dp)
>  {
>         u32 reg;
> -       int timeout_loop = 0;
>
>         analogix_dp_set_analog_power_down(dp, POWER_ALL, 0);
>
> @@ -368,18 +367,7 @@ int analogix_dp_init_analog_func(struct analogix_dp_device *dp)
>         writel(reg, dp->reg_base + ANALOGIX_DP_DEBUG_CTL);
>
>         /* Power up PLL */
> -       if (analogix_dp_get_pll_lock_status(dp) == PLL_UNLOCKED) {
> -               analogix_dp_set_pll_power_down(dp, 0);
> -
> -               while (analogix_dp_get_pll_lock_status(dp) == PLL_UNLOCKED) {
> -                       timeout_loop++;
> -                       if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
> -                               dev_err(dp->dev, "failed to get pll lock status\n");
> -                               return -ETIMEDOUT;
> -                       }
> -                       usleep_range(10, 20);
> -               }
> -       }
> +       analogix_dp_set_pll_power_down(dp, 0);
>
>         /* Enable Serdes FIFO function and Link symbol clock domain module */
>         reg = readl(dp->reg_base + ANALOGIX_DP_FUNC_EN_2);
> --
> 2.39.2
>


Reviewed-by: Robert Foss <rfoss@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 12/14] drm/bridge: analogix_dp: simplify and correct PLL lock checks

[Lucas Stach]

Move the wait loop into its own function, so it doesn't need to be
replicated in multiple locations. Also move the PLL lock checks between
setting the link bandwidth, which may cause the PLL to unlock, and the
MACRO_RST, which needs the PLL to be locked.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 .../drm/bridge/analogix/analogix_dp_core.c    | 34 +++++++------------
 .../drm/bridge/analogix/analogix_dp_core.h    |  7 +---
 .../gpu/drm/bridge/analogix/analogix_dp_reg.c | 12 +++----
 3 files changed, 18 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 736b2ed745e1..7bbc3d8a85df 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -231,7 +231,7 @@ static int analogix_dp_training_pattern_dis(struct analogix_dp_device *dp)
 static int analogix_dp_link_start(struct analogix_dp_device *dp)
 {
 	u8 buf[4];
-	int lane, lane_count, pll_tries, retval;
+	int lane, lane_count, retval;
 
 	lane_count = dp->link_train.lane_count;
 
@@ -243,6 +243,11 @@ static int analogix_dp_link_start(struct analogix_dp_device *dp)
 
 	/* Set link rate and count as you want to establish*/
 	analogix_dp_set_link_bandwidth(dp, dp->link_train.link_rate);
+	retval = analogix_dp_wait_pll_locked(dp);
+	if (retval) {
+		DRM_DEV_ERROR(dp->dev, "Wait for pll lock failed %d\n", retval);
+		return retval;
+	}
 	/*
 	 * MACRO_RST must be applied after the PLL_LOCK to avoid
 	 * the DP inter pair skew issue for at least 10 us
@@ -270,18 +275,6 @@ static int analogix_dp_link_start(struct analogix_dp_device *dp)
 					DP_TRAIN_PRE_EMPH_LEVEL_0;
 	analogix_dp_set_lane_link_training(dp);
 
-	/* Wait for PLL lock */
-	pll_tries = 0;
-	while (analogix_dp_get_pll_lock_status(dp) == PLL_UNLOCKED) {
-		if (pll_tries == DP_TIMEOUT_LOOP_COUNT) {
-			dev_err(dp->dev, "Wait for PLL lock timed out\n");
-			return -ETIMEDOUT;
-		}
-
-		pll_tries++;
-		usleep_range(90, 120);
-	}
-
 	/* Set training pattern 1 */
 	analogix_dp_set_training_pattern(dp, TRAINING_PTN1);
 
@@ -634,9 +627,14 @@ static int analogix_dp_fast_link_train(struct analogix_dp_device *dp)
 {
 	int ret;
 	u8 link_align, link_status[2];
-	enum pll_status status;
 
 	analogix_dp_set_link_bandwidth(dp, dp->link_train.link_rate);
+	ret = analogix_dp_wait_pll_locked(dp);
+	if (ret) {
+		DRM_DEV_ERROR(dp->dev, "Wait for pll lock failed %d\n", ret);
+		return ret;
+	}
+
 	/*
 	 * MACRO_RST must be applied after the PLL_LOCK to avoid
 	 * the DP inter pair skew issue for at least 10 us
@@ -645,14 +643,6 @@ static int analogix_dp_fast_link_train(struct analogix_dp_device *dp)
 	analogix_dp_set_lane_count(dp, dp->link_train.lane_count);
 	analogix_dp_set_lane_link_training(dp);
 
-	ret = readx_poll_timeout(analogix_dp_get_pll_lock_status, dp, status,
-				 status != PLL_UNLOCKED, 120,
-				 120 * DP_TIMEOUT_LOOP_COUNT);
-	if (ret) {
-		DRM_DEV_ERROR(dp->dev, "Wait for pll lock failed %d\n", ret);
-		return ret;
-	}
-
 	/* source Set training pattern 1 */
 	analogix_dp_set_training_pattern(dp, TRAINING_PTN1);
 	/* From DP spec, pattern must be on-screen for a minimum 500us */
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index 382b2f068ab9..774d11574b09 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -95,11 +95,6 @@ enum dynamic_range {
 	CEA
 };
 
-enum pll_status {
-	PLL_UNLOCKED,
-	PLL_LOCKED
-};
-
 enum clock_recovery_m_value_type {
 	CALCULATED_M,
 	REGISTER_M
@@ -191,7 +186,7 @@ void analogix_dp_swreset(struct analogix_dp_device *dp);
 void analogix_dp_config_interrupt(struct analogix_dp_device *dp);
 void analogix_dp_mute_hpd_interrupt(struct analogix_dp_device *dp);
 void analogix_dp_unmute_hpd_interrupt(struct analogix_dp_device *dp);
-enum pll_status analogix_dp_get_pll_lock_status(struct analogix_dp_device *dp);
+int analogix_dp_wait_pll_locked(struct analogix_dp_device *dp);
 void analogix_dp_set_pll_power_down(struct analogix_dp_device *dp, bool enable);
 void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
 				       enum analog_power_block block,
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index e9c643a8b6fc..143a78b1d156 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
@@ -217,15 +217,13 @@ void analogix_dp_unmute_hpd_interrupt(struct analogix_dp_device *dp)
 	writel(reg, dp->reg_base + ANALOGIX_DP_INT_STA_MASK);
 }
 
-enum pll_status analogix_dp_get_pll_lock_status(struct analogix_dp_device *dp)
+int analogix_dp_wait_pll_locked(struct analogix_dp_device *dp)
 {
-	u32 reg;
+	u32 val;
 
-	reg = readl(dp->reg_base + ANALOGIX_DP_DEBUG_CTL);
-	if (reg & PLL_LOCK)
-		return PLL_LOCKED;
-	else
-		return PLL_UNLOCKED;
+	return readl_poll_timeout(dp->reg_base + ANALOGIX_DP_DEBUG_CTL, val,
+				  val & PLL_LOCK, 120,
+				  120 * DP_TIMEOUT_LOOP_COUNT);
 }
 
 void analogix_dp_set_pll_power_down(struct analogix_dp_device *dp, bool enable)
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 12/14] drm/bridge: analogix_dp: simplify and correct PLL lock checks

[Robert Foss]

On Fri, May 3, 2024 at 5:12 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Move the wait loop into its own function, so it doesn't need to be
> replicated in multiple locations. Also move the PLL lock checks between
> setting the link bandwidth, which may cause the PLL to unlock, and the
> MACRO_RST, which needs the PLL to be locked.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  .../drm/bridge/analogix/analogix_dp_core.c    | 34 +++++++------------
>  .../drm/bridge/analogix/analogix_dp_core.h    |  7 +---
>  .../gpu/drm/bridge/analogix/analogix_dp_reg.c | 12 +++----
>  3 files changed, 18 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 736b2ed745e1..7bbc3d8a85df 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -231,7 +231,7 @@ static int analogix_dp_training_pattern_dis(struct analogix_dp_device *dp)
>  static int analogix_dp_link_start(struct analogix_dp_device *dp)
>  {
>         u8 buf[4];
> -       int lane, lane_count, pll_tries, retval;
> +       int lane, lane_count, retval;
>
>         lane_count = dp->link_train.lane_count;
>
> @@ -243,6 +243,11 @@ static int analogix_dp_link_start(struct analogix_dp_device *dp)
>
>         /* Set link rate and count as you want to establish*/
>         analogix_dp_set_link_bandwidth(dp, dp->link_train.link_rate);
> +       retval = analogix_dp_wait_pll_locked(dp);
> +       if (retval) {
> +               DRM_DEV_ERROR(dp->dev, "Wait for pll lock failed %d\n", retval);
> +               return retval;
> +       }
>         /*
>          * MACRO_RST must be applied after the PLL_LOCK to avoid
>          * the DP inter pair skew issue for at least 10 us
> @@ -270,18 +275,6 @@ static int analogix_dp_link_start(struct analogix_dp_device *dp)
>                                         DP_TRAIN_PRE_EMPH_LEVEL_0;
>         analogix_dp_set_lane_link_training(dp);
>
> -       /* Wait for PLL lock */
> -       pll_tries = 0;
> -       while (analogix_dp_get_pll_lock_status(dp) == PLL_UNLOCKED) {
> -               if (pll_tries == DP_TIMEOUT_LOOP_COUNT) {
> -                       dev_err(dp->dev, "Wait for PLL lock timed out\n");
> -                       return -ETIMEDOUT;
> -               }
> -
> -               pll_tries++;
> -               usleep_range(90, 120);
> -       }
> -
>         /* Set training pattern 1 */
>         analogix_dp_set_training_pattern(dp, TRAINING_PTN1);
>
> @@ -634,9 +627,14 @@ static int analogix_dp_fast_link_train(struct analogix_dp_device *dp)
>  {
>         int ret;
>         u8 link_align, link_status[2];
> -       enum pll_status status;
>
>         analogix_dp_set_link_bandwidth(dp, dp->link_train.link_rate);
> +       ret = analogix_dp_wait_pll_locked(dp);
> +       if (ret) {
> +               DRM_DEV_ERROR(dp->dev, "Wait for pll lock failed %d\n", ret);
> +               return ret;
> +       }
> +
>         /*
>          * MACRO_RST must be applied after the PLL_LOCK to avoid
>          * the DP inter pair skew issue for at least 10 us
> @@ -645,14 +643,6 @@ static int analogix_dp_fast_link_train(struct analogix_dp_device *dp)
>         analogix_dp_set_lane_count(dp, dp->link_train.lane_count);
>         analogix_dp_set_lane_link_training(dp);
>
> -       ret = readx_poll_timeout(analogix_dp_get_pll_lock_status, dp, status,
> -                                status != PLL_UNLOCKED, 120,
> -                                120 * DP_TIMEOUT_LOOP_COUNT);
> -       if (ret) {
> -               DRM_DEV_ERROR(dp->dev, "Wait for pll lock failed %d\n", ret);
> -               return ret;
> -       }
> -
>         /* source Set training pattern 1 */
>         analogix_dp_set_training_pattern(dp, TRAINING_PTN1);
>         /* From DP spec, pattern must be on-screen for a minimum 500us */
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> index 382b2f068ab9..774d11574b09 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> @@ -95,11 +95,6 @@ enum dynamic_range {
>         CEA
>  };
>
> -enum pll_status {
> -       PLL_UNLOCKED,
> -       PLL_LOCKED
> -};
> -
>  enum clock_recovery_m_value_type {
>         CALCULATED_M,
>         REGISTER_M
> @@ -191,7 +186,7 @@ void analogix_dp_swreset(struct analogix_dp_device *dp);
>  void analogix_dp_config_interrupt(struct analogix_dp_device *dp);
>  void analogix_dp_mute_hpd_interrupt(struct analogix_dp_device *dp);
>  void analogix_dp_unmute_hpd_interrupt(struct analogix_dp_device *dp);
> -enum pll_status analogix_dp_get_pll_lock_status(struct analogix_dp_device *dp);
> +int analogix_dp_wait_pll_locked(struct analogix_dp_device *dp);
>  void analogix_dp_set_pll_power_down(struct analogix_dp_device *dp, bool enable);
>  void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
>                                        enum analog_power_block block,
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index e9c643a8b6fc..143a78b1d156 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -217,15 +217,13 @@ void analogix_dp_unmute_hpd_interrupt(struct analogix_dp_device *dp)
>         writel(reg, dp->reg_base + ANALOGIX_DP_INT_STA_MASK);
>  }
>
> -enum pll_status analogix_dp_get_pll_lock_status(struct analogix_dp_device *dp)
> +int analogix_dp_wait_pll_locked(struct analogix_dp_device *dp)
>  {
> -       u32 reg;
> +       u32 val;
>
> -       reg = readl(dp->reg_base + ANALOGIX_DP_DEBUG_CTL);
> -       if (reg & PLL_LOCK)
> -               return PLL_LOCKED;
> -       else
> -               return PLL_UNLOCKED;
> +       return readl_poll_timeout(dp->reg_base + ANALOGIX_DP_DEBUG_CTL, val,
> +                                 val & PLL_LOCK, 120,
> +                                 120 * DP_TIMEOUT_LOOP_COUNT);
>  }
>
>  void analogix_dp_set_pll_power_down(struct analogix_dp_device *dp, bool enable)
> --
> 2.39.2
>

Reviewed-by: Robert Foss <rfoss@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 13/14] drm/bridge: analogix_dp: only read AUX status when an error occured

[Lucas Stach]

All AUX error responses raise the AUX_ERR interrupt, so there is no
need to read the AUX status register in normal operation. Only read
the status when an error occured and we can expect a different status
than OK.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index 143a78b1d156..0f016dca9f3d 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
@@ -924,7 +924,6 @@ ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
 			     struct drm_dp_aux_msg *msg)
 {
 	u32 reg;
-	u32 status_reg;
 	u8 *buffer = msg->buffer;
 	unsigned int i;
 	int ret;
@@ -1011,12 +1010,14 @@ ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
 
 	/* Clear interrupt source for AUX CH access error */
 	reg = readl(dp->reg_base + ANALOGIX_DP_INT_STA);
-	status_reg = readl(dp->reg_base + ANALOGIX_DP_AUX_CH_STA);
-	if ((reg & AUX_ERR) || (status_reg & AUX_STATUS_MASK)) {
+	if ((reg & AUX_ERR)) {
+		u32 aux_status = readl(dp->reg_base + ANALOGIX_DP_AUX_CH_STA) &
+				 AUX_STATUS_MASK;
+
 		writel(AUX_ERR, dp->reg_base + ANALOGIX_DP_INT_STA);
 
 		dev_warn(dp->dev, "AUX CH error happened: %#x (%d)\n",
-			 status_reg & AUX_STATUS_MASK, !!(reg & AUX_ERR));
+			 aux_status, !!(reg & AUX_ERR));
 		goto aux_error;
 	}
 
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 13/14] drm/bridge: analogix_dp: only read AUX status when an error occured

[Robert Foss]

On Fri, May 3, 2024 at 5:13 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> All AUX error responses raise the AUX_ERR interrupt, so there is no
> need to read the AUX status register in normal operation. Only read
> the status when an error occured and we can expect a different status
> than OK.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index 143a78b1d156..0f016dca9f3d 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -924,7 +924,6 @@ ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
>                              struct drm_dp_aux_msg *msg)
>  {
>         u32 reg;
> -       u32 status_reg;
>         u8 *buffer = msg->buffer;
>         unsigned int i;
>         int ret;
> @@ -1011,12 +1010,14 @@ ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
>
>         /* Clear interrupt source for AUX CH access error */
>         reg = readl(dp->reg_base + ANALOGIX_DP_INT_STA);
> -       status_reg = readl(dp->reg_base + ANALOGIX_DP_AUX_CH_STA);
> -       if ((reg & AUX_ERR) || (status_reg & AUX_STATUS_MASK)) {
> +       if ((reg & AUX_ERR)) {
> +               u32 aux_status = readl(dp->reg_base + ANALOGIX_DP_AUX_CH_STA) &
> +                                AUX_STATUS_MASK;
> +
>                 writel(AUX_ERR, dp->reg_base + ANALOGIX_DP_INT_STA);
>
>                 dev_warn(dp->dev, "AUX CH error happened: %#x (%d)\n",
> -                        status_reg & AUX_STATUS_MASK, !!(reg & AUX_ERR));
> +                        aux_status, !!(reg & AUX_ERR));
>                 goto aux_error;
>         }
>
> --
> 2.39.2
>

Reviewed-by: Robert Foss <rfoss@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 14/14] drm/bridge: analogix_dp: handle AUX transfer timeouts

[Lucas Stach]

Timeouts on the AUX bus are to be expected in certain normal operating
conditions. There is no need to raise an error log or re-initialize the
whole AUX state machine. Simply acknowledge the AUX_ERR interrupt and
let upper layers know about the timeout.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 3 +++
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index 0f016dca9f3d..3afc73c858c4 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
@@ -1016,6 +1016,9 @@ ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
 
 		writel(AUX_ERR, dp->reg_base + ANALOGIX_DP_INT_STA);
 
+		if (aux_status == AUX_STATUS_TIMEOUT_ERROR)
+			return -ETIMEDOUT;
+
 		dev_warn(dp->dev, "AUX CH error happened: %#x (%d)\n",
 			 aux_status, !!(reg & AUX_ERR));
 		goto aux_error;
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
index e284ee8da58b..12735139046c 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
@@ -361,6 +361,15 @@
 /* ANALOGIX_DP_AUX_CH_STA */
 #define AUX_BUSY				(0x1 << 4)
 #define AUX_STATUS_MASK				(0xf << 0)
+#define AUX_STATUS_OK				(0x0 << 0)
+#define AUX_STATUS_NACK_ERROR			(0x1 << 0)
+#define AUX_STATUS_TIMEOUT_ERROR		(0x2 << 0)
+#define AUX_STATUS_UNKNOWN_ERROR		(0x3 << 0)
+#define AUX_STATUS_MUCH_DEFER_ERROR		(0x4 << 0)
+#define AUX_STATUS_TX_SHORT_ERROR		(0x5 << 0)
+#define AUX_STATUS_RX_SHORT_ERROR		(0x6 << 0)
+#define AUX_STATUS_NACK_WITHOUT_M_ERROR		(0x7 << 0)
+#define AUX_STATUS_I2C_NACK_ERROR		(0x8 << 0)
 
 /* ANALOGIX_DP_AUX_CH_DEFER_CTL */
 #define DEFER_CTRL_EN				(0x1 << 7)
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 14/14] drm/bridge: analogix_dp: handle AUX transfer timeouts

[Robert Foss]

On Fri, May 3, 2024 at 5:12 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Timeouts on the AUX bus are to be expected in certain normal operating
> conditions. There is no need to raise an error log or re-initialize the
> whole AUX state machine. Simply acknowledge the AUX_ERR interrupt and
> let upper layers know about the timeout.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 3 +++
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h | 9 +++++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index 0f016dca9f3d..3afc73c858c4 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -1016,6 +1016,9 @@ ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
>
>                 writel(AUX_ERR, dp->reg_base + ANALOGIX_DP_INT_STA);
>
> +               if (aux_status == AUX_STATUS_TIMEOUT_ERROR)
> +                       return -ETIMEDOUT;
> +
>                 dev_warn(dp->dev, "AUX CH error happened: %#x (%d)\n",
>                          aux_status, !!(reg & AUX_ERR));
>                 goto aux_error;
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
> index e284ee8da58b..12735139046c 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
> @@ -361,6 +361,15 @@
>  /* ANALOGIX_DP_AUX_CH_STA */
>  #define AUX_BUSY                               (0x1 << 4)
>  #define AUX_STATUS_MASK                                (0xf << 0)
> +#define AUX_STATUS_OK                          (0x0 << 0)
> +#define AUX_STATUS_NACK_ERROR                  (0x1 << 0)
> +#define AUX_STATUS_TIMEOUT_ERROR               (0x2 << 0)
> +#define AUX_STATUS_UNKNOWN_ERROR               (0x3 << 0)
> +#define AUX_STATUS_MUCH_DEFER_ERROR            (0x4 << 0)
> +#define AUX_STATUS_TX_SHORT_ERROR              (0x5 << 0)
> +#define AUX_STATUS_RX_SHORT_ERROR              (0x6 << 0)
> +#define AUX_STATUS_NACK_WITHOUT_M_ERROR                (0x7 << 0)
> +#define AUX_STATUS_I2C_NACK_ERROR              (0x8 << 0)
>
>  /* ANALOGIX_DP_AUX_CH_DEFER_CTL */
>  #define DEFER_CTRL_EN                          (0x1 << 7)
> --
> 2.39.2
>

Reviewed-by: Robert Foss <rfoss@kernel.org>

This series has a few checkpath --strict warnings, could you fix those
and I'll merge v2?


Rob.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 00/14] improve Analogix DP AUX channel handling

[Robert Foss]

On Fri, May 3, 2024 at 5:12 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Currently the AUX channel support in the Analogix DP driver is severely
> limited as the AUX block of the bridge is only initialized when the video
> link is to be enabled. This is okay for the purposes of link training,
> but does not allow to detect displays by probing for EDID. This series
> reworks the driver to allow AUX transactions before the video link is
> active.
>
> As this requires to rework some of the controller initialization and
> also handling of both internal and external clocks, the series includes
> quite a few changes to add better runtime PM handling.
>
> Lucas Stach (14):
>   drm/bridge: analogix_dp: remove unused platform power_on_end callback
>   drm/rockchip: analogix_dp: add runtime PM handling
>   drm/bridge: analogix_dp: register AUX bus after enabling runtime PM
>   drm/bridge: analogix_dp: handle clock via runtime PM
>   drm/bridge: analogix_dp: remove unused analogix_dp_remove
>   drm/bridge: analogix_dp: remove clk handling from
>     analogix_dp_set_bridge
>   drm/bridge: analogix_dp: move platform and PHY power handling into
>     runtime PM
>   drm/bridge: analogix_dp: move basic controller init into runtime PM
>   drm/bridge: analogix_dp: remove PLL lock check from
>     analogix_dp_config_video
>   drm/bridge: analogix_dp: move macro reset after link bandwidth setting
>   drm/bridge: analogix_dp: don't wait for PLL lock too early
>   drm/bridge: analogix_dp: simplify and correct PLL lock checks
>   drm/bridge: analogix_dp: only read AUX status when an error occured
>   drm/bridge: analogix_dp: handle AUX transfer timeouts
>
>  .../drm/bridge/analogix/analogix_dp_core.c    | 196 ++++++++----------
>  .../drm/bridge/analogix/analogix_dp_core.h    |   7 +-
>  .../gpu/drm/bridge/analogix/analogix_dp_reg.c |  38 ++--
>  .../gpu/drm/bridge/analogix/analogix_dp_reg.h |   9 +
>  drivers/gpu/drm/exynos/exynos_dp.c            |   5 +-
>  .../gpu/drm/rockchip/analogix_dp-rockchip.c   |  26 +--
>  include/drm/bridge/analogix_dp.h              |   4 +-
>  7 files changed, 120 insertions(+), 165 deletions(-)
>
> --
> 2.39.2
>

There are some checkpatch --strict warnings, and the patch 10/14 does
not apply. Other than that the series looks very good.

Maybe rebase on drm-misc/drm-misc-next, fix the applicable checkpatch
--strict warnings and send a new version of this series. Then the last
patches can be reviewed.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 00/14] improve Analogix DP AUX channel handling

[Lucas Stach]

Hi Robert,

Am Dienstag, dem 07.05.2024 um 15:10 +0200 schrieb Robert Foss:
> On Fri, May 3, 2024 at 5:12 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> > 
> > Currently the AUX channel support in the Analogix DP driver is severely
> > limited as the AUX block of the bridge is only initialized when the video
> > link is to be enabled. This is okay for the purposes of link training,
> > but does not allow to detect displays by probing for EDID. This series
> > reworks the driver to allow AUX transactions before the video link is
> > active.
> > 
> > As this requires to rework some of the controller initialization and
> > also handling of both internal and external clocks, the series includes
> > quite a few changes to add better runtime PM handling.
> > 
> > Lucas Stach (14):
> >   drm/bridge: analogix_dp: remove unused platform power_on_end callback
> >   drm/rockchip: analogix_dp: add runtime PM handling
> >   drm/bridge: analogix_dp: register AUX bus after enabling runtime PM
> >   drm/bridge: analogix_dp: handle clock via runtime PM
> >   drm/bridge: analogix_dp: remove unused analogix_dp_remove
> >   drm/bridge: analogix_dp: remove clk handling from
> >     analogix_dp_set_bridge
> >   drm/bridge: analogix_dp: move platform and PHY power handling into
> >     runtime PM
> >   drm/bridge: analogix_dp: move basic controller init into runtime PM
> >   drm/bridge: analogix_dp: remove PLL lock check from
> >     analogix_dp_config_video
> >   drm/bridge: analogix_dp: move macro reset after link bandwidth setting
> >   drm/bridge: analogix_dp: don't wait for PLL lock too early
> >   drm/bridge: analogix_dp: simplify and correct PLL lock checks
> >   drm/bridge: analogix_dp: only read AUX status when an error occured
> >   drm/bridge: analogix_dp: handle AUX transfer timeouts
> > 
> >  .../drm/bridge/analogix/analogix_dp_core.c    | 196 ++++++++----------
> >  .../drm/bridge/analogix/analogix_dp_core.h    |   7 +-
> >  .../gpu/drm/bridge/analogix/analogix_dp_reg.c |  38 ++--
> >  .../gpu/drm/bridge/analogix/analogix_dp_reg.h |   9 +
> >  drivers/gpu/drm/exynos/exynos_dp.c            |   5 +-
> >  .../gpu/drm/rockchip/analogix_dp-rockchip.c   |  26 +--
> >  include/drm/bridge/analogix_dp.h              |   4 +-
> >  7 files changed, 120 insertions(+), 165 deletions(-)
> > 
> > --
> > 2.39.2
> > 
> 
> There are some checkpatch --strict warnings, and the patch 10/14 does
> not apply. Other than that the series looks very good.
> 
> Maybe rebase on drm-misc/drm-misc-next, fix the applicable checkpatch
> --strict warnings and send a new version of this series. Then the last
> patches can be reviewed.

Thanks for the review so far. Patch 10/14 probably doesn't apply, as I
based the series on some patches I sent earlier [1]. Maybe you are also
willing to take a look at those, so I could squash them into a single
series for the resend?

Regards,
Lucas

[1] https://lore.kernel.org/dri-devel/20240318203925.2837689-1-l.stach@pengutronix.de/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 00/14] improve Analogix DP AUX channel handling

[Heiko Stübner]

Am Freitag, 3. Mai 2024, 17:11:15 CEST schrieb Lucas Stach:
> Currently the AUX channel support in the Analogix DP driver is severely
> limited as the AUX block of the bridge is only initialized when the video
> link is to be enabled. This is okay for the purposes of link training,
> but does not allow to detect displays by probing for EDID. This series
> reworks the driver to allow AUX transactions before the video link is
> active.
> 
> As this requires to rework some of the controller initialization and
> also handling of both internal and external clocks, the series includes
> quite a few changes to add better runtime PM handling.
> 
> Lucas Stach (14):
>   drm/bridge: analogix_dp: remove unused platform power_on_end callback
>   drm/rockchip: analogix_dp: add runtime PM handling
>   drm/bridge: analogix_dp: register AUX bus after enabling runtime PM
>   drm/bridge: analogix_dp: handle clock via runtime PM
>   drm/bridge: analogix_dp: remove unused analogix_dp_remove
>   drm/bridge: analogix_dp: remove clk handling from
>     analogix_dp_set_bridge
>   drm/bridge: analogix_dp: move platform and PHY power handling into
>     runtime PM
>   drm/bridge: analogix_dp: move basic controller init into runtime PM
>   drm/bridge: analogix_dp: remove PLL lock check from
>     analogix_dp_config_video
>   drm/bridge: analogix_dp: move macro reset after link bandwidth setting
>   drm/bridge: analogix_dp: don't wait for PLL lock too early
>   drm/bridge: analogix_dp: simplify and correct PLL lock checks
>   drm/bridge: analogix_dp: only read AUX status when an error occured
>   drm/bridge: analogix_dp: handle AUX transfer timeouts

my knowledge of Analgix-dp internals is limited, but at least both
rk3288-veyron and rk3399 gru still had working displays with this series
applied and both device classes using the analogix-dp for their main
display.

So on rk3288-veyron and rk3399-gru
Tested-by: Heiko Stuebner <heiko@sntech.de>



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel