[PATCH RESEND v3 0/3] Update STM DSI PHY driver

[PATCH RESEND v3 0/3] Update STM DSI PHY driver

[Raphael Gallais-Pou]

This patch series aims to add several features of the dw-mipi-dsi phy
driver that are missing or need to be updated.

First patch update a PM macro.

Second patch adds runtime PM functionality to the driver.

Third patch adds a clock provider generated by the PHY itself.  As
explained in the commit log of the second patch, a clock declaration is
missing.  Since this clock is parent of 'dsi_k', it leads to an orphan
clock.  Most importantly this patch is an anticipation for future
versions of the DSI PHY, and its inclusion within the display subsystem
and the DRM framework.

Last patch fixes a corner effect introduced previously.  Since 'dsi' and
'dsi_k' are gated by the same bit on the same register, both reference
work as peripheral clock in the device-tree.

---
Changes in v3-resend:
	- Removed last patch as it has been merged
https://lore.kernel.org/lkml/bf49f4c9-9e81-4c91-972d-13782d996aaa@foss.st.com/

Changes in v3:
	- Fix smatch warning (disable dsi->pclk when clk_register fails)

Changes in v2:
	- Added patch 1/4 to use SYSTEM_SLEEP_PM_OPS instead of old macro
	  and removed __maybe_used for accordingly
	- Changed SET_RUNTIME_PM_OPS to RUNTIME_PM_OPS

Raphael Gallais-Pou (3):
  drm/stm: dsi: use new SYSTEM_SLEEP_PM_OPS() macro
  drm/stm: dsi: expose DSI PHY internal clock

Yannick Fertre (1):
  drm/stm: dsi: add pm runtime ops

 drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 279 ++++++++++++++++++++++----
 1 file changed, 238 insertions(+), 41 deletions(-)

-- 
2.25.1


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

[PATCH RESEND v3 1/3] drm/stm: dsi: use new SYSTEM_SLEEP_PM_OPS() macro

[Raphael Gallais-Pou]

Use RUNTIME_PM_OPS() instead of the old SET_SYSTEM_SLEEP_PM_OPS().
This means we don't need  __maybe_unused on the functions.

Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
---
 drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
index d5f8c923d7bc..b1aee43d51e9 100644
--- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
+++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
@@ -544,7 +544,7 @@ static void dw_mipi_dsi_stm_remove(struct platform_device *pdev)
 	regulator_disable(dsi->vdd_supply);
 }
 
-static int __maybe_unused dw_mipi_dsi_stm_suspend(struct device *dev)
+static int dw_mipi_dsi_stm_suspend(struct device *dev)
 {
 	struct dw_mipi_dsi_stm *dsi = dw_mipi_dsi_stm_plat_data.priv_data;
 
@@ -556,7 +556,7 @@ static int __maybe_unused dw_mipi_dsi_stm_suspend(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused dw_mipi_dsi_stm_resume(struct device *dev)
+static int dw_mipi_dsi_stm_resume(struct device *dev)
 {
 	struct dw_mipi_dsi_stm *dsi = dw_mipi_dsi_stm_plat_data.priv_data;
 	int ret;
@@ -580,8 +580,8 @@ static int __maybe_unused dw_mipi_dsi_stm_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops dw_mipi_dsi_stm_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(dw_mipi_dsi_stm_suspend,
-				dw_mipi_dsi_stm_resume)
+	SYSTEM_SLEEP_PM_OPS(dw_mipi_dsi_stm_suspend,
+			    dw_mipi_dsi_stm_resume)
 };
 
 static struct platform_driver dw_mipi_dsi_stm_driver = {
-- 
2.25.1


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

[PATCH RESEND v3 2/3] drm/stm: dsi: add pm runtime ops

[Raphael Gallais-Pou]

From: Yannick Fertre <yannick.fertre@foss.st.com>

Update control of clocks and supply thanks to the PM runtime
mechanism to avoid kernel crash during a system suspend.

Signed-off-by: Yannick Fertre <yannick.fertre@foss.st.com>
Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
---
Changes in v2:
	- Changed SET_RUNTIME_PM_OPS to RUNTIME_PM_OPS and removed
	__maybe_unused
---
 drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
index b1aee43d51e9..82fff9e84345 100644
--- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
+++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
@@ -11,6 +11,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 
 #include <video/mipi_display.h>
@@ -77,6 +78,7 @@ enum dsi_color {
 struct dw_mipi_dsi_stm {
 	void __iomem *base;
 	struct clk *pllref_clk;
+	struct clk *pclk;
 	struct dw_mipi_dsi *dsi;
 	u32 hw_version;
 	int lane_min_kbps;
@@ -443,7 +445,6 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct dw_mipi_dsi_stm *dsi;
-	struct clk *pclk;
 	int ret;
 
 	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
@@ -483,21 +484,21 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
 		goto err_clk_get;
 	}
 
-	pclk = devm_clk_get(dev, "pclk");
-	if (IS_ERR(pclk)) {
-		ret = PTR_ERR(pclk);
+	dsi->pclk = devm_clk_get(dev, "pclk");
+	if (IS_ERR(dsi->pclk)) {
+		ret = PTR_ERR(dsi->pclk);
 		DRM_ERROR("Unable to get peripheral clock: %d\n", ret);
 		goto err_dsi_probe;
 	}
 
-	ret = clk_prepare_enable(pclk);
+	ret = clk_prepare_enable(dsi->pclk);
 	if (ret) {
 		DRM_ERROR("%s: Failed to enable peripheral clk\n", __func__);
 		goto err_dsi_probe;
 	}
 
 	dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION;
-	clk_disable_unprepare(pclk);
+	clk_disable_unprepare(dsi->pclk);
 
 	if (dsi->hw_version != HWVER_130 && dsi->hw_version != HWVER_131) {
 		ret = -ENODEV;
@@ -551,6 +552,7 @@ static int dw_mipi_dsi_stm_suspend(struct device *dev)
 	DRM_DEBUG_DRIVER("\n");
 
 	clk_disable_unprepare(dsi->pllref_clk);
+	clk_disable_unprepare(dsi->pclk);
 	regulator_disable(dsi->vdd_supply);
 
 	return 0;
@@ -569,8 +571,16 @@ static int dw_mipi_dsi_stm_resume(struct device *dev)
 		return ret;
 	}
 
+	ret = clk_prepare_enable(dsi->pclk);
+	if (ret) {
+		regulator_disable(dsi->vdd_supply);
+		DRM_ERROR("Failed to enable pclk: %d\n", ret);
+		return ret;
+	}
+
 	ret = clk_prepare_enable(dsi->pllref_clk);
 	if (ret) {
+		clk_disable_unprepare(dsi->pclk);
 		regulator_disable(dsi->vdd_supply);
 		DRM_ERROR("Failed to enable pllref_clk: %d\n", ret);
 		return ret;
@@ -582,6 +592,8 @@ static int dw_mipi_dsi_stm_resume(struct device *dev)
 static const struct dev_pm_ops dw_mipi_dsi_stm_pm_ops = {
 	SYSTEM_SLEEP_PM_OPS(dw_mipi_dsi_stm_suspend,
 			    dw_mipi_dsi_stm_resume)
+	RUNTIME_PM_OPS(dw_mipi_dsi_stm_suspend,
+		       dw_mipi_dsi_stm_resume, NULL)
 };
 
 static struct platform_driver dw_mipi_dsi_stm_driver = {
-- 
2.25.1


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

[PATCH RESEND v3 3/3] drm/stm: dsi: expose DSI PHY internal clock

[Raphael Gallais-Pou]

	DSISRC __________
	               __\_
	              |    \
	pll4_p_ck   ->|  1  |____dsi_k
	ck_dsi_phy  ->|  0  |
	              |____/

A DSI clock is missing in the clock framework. Looking at the
clk_summary, it appears that 'ck_dsi_phy' is not implemented. Since the
DSI kernel clock is based on the internal DSI pll. The common clock
driver can not directly expose this 'ck_dsi_phy' clock because it does
not contain any common registers with the DSI. Thus it needs to be done
directly within the DSI phy driver.

Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
---
Changes in v3:
	- Fix smatch warning:
	.../dw_mipi_dsi-stm.c:719 dw_mipi_dsi_stm_probe() warn: 'dsi->pclk'
	from clk_prepare_enable() not released on lines: 719.
---
 drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 247 ++++++++++++++++++++++----
 1 file changed, 216 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
index 82fff9e84345..b20123854c4a 100644
--- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
+++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
@@ -7,7 +7,9 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/iopoll.h>
+#include <linux/kernel.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -77,9 +79,12 @@ enum dsi_color {
 
 struct dw_mipi_dsi_stm {
 	void __iomem *base;
+	struct device *dev;
 	struct clk *pllref_clk;
 	struct clk *pclk;
+	struct clk_hw txbyte_clk;
 	struct dw_mipi_dsi *dsi;
+	struct dw_mipi_dsi_plat_data pdata;
 	u32 hw_version;
 	int lane_min_kbps;
 	int lane_max_kbps;
@@ -196,29 +201,198 @@ static int dsi_pll_get_params(struct dw_mipi_dsi_stm *dsi,
 	return 0;
 }
 
-static int dw_mipi_dsi_phy_init(void *priv_data)
+#define clk_to_dw_mipi_dsi_stm(clk) \
+	container_of(clk, struct dw_mipi_dsi_stm, txbyte_clk)
+
+static void dw_mipi_dsi_clk_disable(struct clk_hw *clk)
 {
-	struct dw_mipi_dsi_stm *dsi = priv_data;
+	struct dw_mipi_dsi_stm *dsi = clk_to_dw_mipi_dsi_stm(clk);
+
+	DRM_DEBUG_DRIVER("\n");
+
+	/* Disable the DSI PLL */
+	dsi_clear(dsi, DSI_WRPCR, WRPCR_PLLEN);
+
+	/* Disable the regulator */
+	dsi_clear(dsi, DSI_WRPCR, WRPCR_REGEN | WRPCR_BGREN);
+}
+
+static int dw_mipi_dsi_clk_enable(struct clk_hw *clk)
+{
+	struct dw_mipi_dsi_stm *dsi = clk_to_dw_mipi_dsi_stm(clk);
 	u32 val;
 	int ret;
 
+	DRM_DEBUG_DRIVER("\n");
+
 	/* Enable the regulator */
 	dsi_set(dsi, DSI_WRPCR, WRPCR_REGEN | WRPCR_BGREN);
-	ret = readl_poll_timeout(dsi->base + DSI_WISR, val, val & WISR_RRS,
-				 SLEEP_US, TIMEOUT_US);
+	ret = readl_poll_timeout_atomic(dsi->base + DSI_WISR, val, val & WISR_RRS,
+					SLEEP_US, TIMEOUT_US);
 	if (ret)
 		DRM_DEBUG_DRIVER("!TIMEOUT! waiting REGU, let's continue\n");
 
 	/* Enable the DSI PLL & wait for its lock */
 	dsi_set(dsi, DSI_WRPCR, WRPCR_PLLEN);
-	ret = readl_poll_timeout(dsi->base + DSI_WISR, val, val & WISR_PLLLS,
-				 SLEEP_US, TIMEOUT_US);
+	ret = readl_poll_timeout_atomic(dsi->base + DSI_WISR, val, val & WISR_PLLLS,
+					SLEEP_US, TIMEOUT_US);
 	if (ret)
 		DRM_DEBUG_DRIVER("!TIMEOUT! waiting PLL, let's continue\n");
 
 	return 0;
 }
 
+static int dw_mipi_dsi_clk_is_enabled(struct clk_hw *hw)
+{
+	struct dw_mipi_dsi_stm *dsi = clk_to_dw_mipi_dsi_stm(hw);
+
+	return dsi_read(dsi, DSI_WRPCR) & WRPCR_PLLEN;
+}
+
+static unsigned long dw_mipi_dsi_clk_recalc_rate(struct clk_hw *hw,
+						 unsigned long parent_rate)
+{
+	struct dw_mipi_dsi_stm *dsi = clk_to_dw_mipi_dsi_stm(hw);
+	unsigned int idf, ndiv, odf, pll_in_khz, pll_out_khz;
+	u32 val;
+
+	DRM_DEBUG_DRIVER("\n");
+
+	pll_in_khz = (unsigned int)(parent_rate / 1000);
+
+	val = dsi_read(dsi, DSI_WRPCR);
+
+	idf = (val & WRPCR_IDF) >> 11;
+	if (!idf)
+		idf = 1;
+	ndiv = (val & WRPCR_NDIV) >> 2;
+	odf = int_pow(2, (val & WRPCR_ODF) >> 16);
+
+	/* Get the adjusted pll out value */
+	pll_out_khz = dsi_pll_get_clkout_khz(pll_in_khz, idf, ndiv, odf);
+
+	return (unsigned long)pll_out_khz * 1000;
+}
+
+static long dw_mipi_dsi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+				       unsigned long *parent_rate)
+{
+	struct dw_mipi_dsi_stm *dsi = clk_to_dw_mipi_dsi_stm(hw);
+	unsigned int idf, ndiv, odf, pll_in_khz, pll_out_khz;
+	int ret;
+
+	DRM_DEBUG_DRIVER("\n");
+
+	pll_in_khz = (unsigned int)(*parent_rate / 1000);
+
+	/* Compute best pll parameters */
+	idf = 0;
+	ndiv = 0;
+	odf = 0;
+
+	ret = dsi_pll_get_params(dsi, pll_in_khz, rate / 1000,
+				 &idf, &ndiv, &odf);
+	if (ret)
+		DRM_WARN("Warning dsi_pll_get_params(): bad params\n");
+
+	/* Get the adjusted pll out value */
+	pll_out_khz = dsi_pll_get_clkout_khz(pll_in_khz, idf, ndiv, odf);
+
+	return pll_out_khz * 1000;
+}
+
+static int dw_mipi_dsi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+				    unsigned long parent_rate)
+{
+	struct dw_mipi_dsi_stm *dsi = clk_to_dw_mipi_dsi_stm(hw);
+	unsigned int idf, ndiv, odf, pll_in_khz, pll_out_khz;
+	int ret;
+	u32 val;
+
+	DRM_DEBUG_DRIVER("\n");
+
+	pll_in_khz = (unsigned int)(parent_rate / 1000);
+
+	/* Compute best pll parameters */
+	idf = 0;
+	ndiv = 0;
+	odf = 0;
+
+	ret = dsi_pll_get_params(dsi, pll_in_khz, rate / 1000, &idf, &ndiv, &odf);
+	if (ret)
+		DRM_WARN("Warning dsi_pll_get_params(): bad params\n");
+
+	/* Get the adjusted pll out value */
+	pll_out_khz = dsi_pll_get_clkout_khz(pll_in_khz, idf, ndiv, odf);
+
+	/* Set the PLL division factors */
+	dsi_update_bits(dsi, DSI_WRPCR,	WRPCR_NDIV | WRPCR_IDF | WRPCR_ODF,
+			(ndiv << 2) | (idf << 11) | ((ffs(odf) - 1) << 16));
+
+	/* Compute uix4 & set the bit period in high-speed mode */
+	val = 4000000 / pll_out_khz;
+	dsi_update_bits(dsi, DSI_WPCR0, WPCR0_UIX4, val);
+
+	return 0;
+}
+
+static void dw_mipi_dsi_clk_unregister(void *data)
+{
+	struct dw_mipi_dsi_stm *dsi = data;
+
+	DRM_DEBUG_DRIVER("\n");
+
+	of_clk_del_provider(dsi->dev->of_node);
+	clk_hw_unregister(&dsi->txbyte_clk);
+}
+
+static const struct clk_ops dw_mipi_dsi_stm_clk_ops = {
+	.enable = dw_mipi_dsi_clk_enable,
+	.disable = dw_mipi_dsi_clk_disable,
+	.is_enabled = dw_mipi_dsi_clk_is_enabled,
+	.recalc_rate = dw_mipi_dsi_clk_recalc_rate,
+	.round_rate = dw_mipi_dsi_clk_round_rate,
+	.set_rate = dw_mipi_dsi_clk_set_rate,
+};
+
+static struct clk_init_data cdata_init = {
+	.name = "ck_dsi_phy",
+	.ops = &dw_mipi_dsi_stm_clk_ops,
+	.parent_names = (const char * []) {"ck_hse"},
+	.num_parents = 1,
+};
+
+static int dw_mipi_dsi_clk_register(struct dw_mipi_dsi_stm *dsi,
+				    struct device *dev)
+{
+	struct device_node *node = dev->of_node;
+	int ret;
+
+	DRM_DEBUG_DRIVER("Registering clk\n");
+
+	dsi->txbyte_clk.init = &cdata_init;
+
+	ret = clk_hw_register(dev, &dsi->txbyte_clk);
+	if (ret)
+		return ret;
+
+	ret = of_clk_add_hw_provider(node, of_clk_hw_simple_get,
+				     &dsi->txbyte_clk);
+	if (ret)
+		clk_hw_unregister(&dsi->txbyte_clk);
+
+	return ret;
+}
+
+static int dw_mipi_dsi_phy_init(void *priv_data)
+{
+	struct dw_mipi_dsi_stm *dsi = priv_data;
+	int ret;
+
+	ret = clk_prepare_enable(dsi->txbyte_clk.clk);
+	return ret;
+}
+
 static void dw_mipi_dsi_phy_power_on(void *priv_data)
 {
 	struct dw_mipi_dsi_stm *dsi = priv_data;
@@ -235,6 +409,8 @@ static void dw_mipi_dsi_phy_power_off(void *priv_data)
 
 	DRM_DEBUG_DRIVER("\n");
 
+	clk_disable_unprepare(dsi->txbyte_clk.clk);
+
 	/* Disable the DSI wrapper */
 	dsi_clear(dsi, DSI_WCR, WCR_DSIEN);
 }
@@ -245,9 +421,8 @@ dw_mipi_dsi_get_lane_mbps(void *priv_data, const struct drm_display_mode *mode,
 			  unsigned int *lane_mbps)
 {
 	struct dw_mipi_dsi_stm *dsi = priv_data;
-	unsigned int idf, ndiv, odf, pll_in_khz, pll_out_khz;
+	unsigned int pll_in_khz, pll_out_khz;
 	int ret, bpp;
-	u32 val;
 
 	pll_in_khz = (unsigned int)(clk_get_rate(dsi->pllref_clk) / 1000);
 
@@ -268,25 +443,10 @@ dw_mipi_dsi_get_lane_mbps(void *priv_data, const struct drm_display_mode *mode,
 		DRM_WARN("Warning min phy mbps is used\n");
 	}
 
-	/* Compute best pll parameters */
-	idf = 0;
-	ndiv = 0;
-	odf = 0;
-	ret = dsi_pll_get_params(dsi, pll_in_khz, pll_out_khz,
-				 &idf, &ndiv, &odf);
+	ret = clk_set_rate((dsi->txbyte_clk.clk), pll_out_khz * 1000);
 	if (ret)
-		DRM_WARN("Warning dsi_pll_get_params(): bad params\n");
-
-	/* Get the adjusted pll out value */
-	pll_out_khz = dsi_pll_get_clkout_khz(pll_in_khz, idf, ndiv, odf);
-
-	/* Set the PLL division factors */
-	dsi_update_bits(dsi, DSI_WRPCR,	WRPCR_NDIV | WRPCR_IDF | WRPCR_ODF,
-			(ndiv << 2) | (idf << 11) | ((ffs(odf) - 1) << 16));
-
-	/* Compute uix4 & set the bit period in high-speed mode */
-	val = 4000000 / pll_out_khz;
-	dsi_update_bits(dsi, DSI_WPCR0, WPCR0_UIX4, val);
+		DRM_DEBUG_DRIVER("ERROR Could not set rate of %d to %s clk->name",
+				 pll_out_khz, clk_hw_get_name(&dsi->txbyte_clk));
 
 	/* Select video mode by resetting DSIM bit */
 	dsi_clear(dsi, DSI_WCFGR, WCFGR_DSIM);
@@ -445,6 +605,7 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct dw_mipi_dsi_stm *dsi;
+	const struct dw_mipi_dsi_plat_data *pdata = of_device_get_match_data(dev);
 	int ret;
 
 	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
@@ -514,18 +675,41 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
 		dsi->lane_max_kbps *= 2;
 	}
 
-	dw_mipi_dsi_stm_plat_data.base = dsi->base;
-	dw_mipi_dsi_stm_plat_data.priv_data = dsi;
+	dsi->pdata = *pdata;
+	dsi->pdata.base = dsi->base;
+	dsi->pdata.priv_data = dsi;
+
+	dsi->pdata.max_data_lanes = 2;
+	dsi->pdata.phy_ops = &dw_mipi_dsi_stm_phy_ops;
 
 	platform_set_drvdata(pdev, dsi);
 
-	dsi->dsi = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
+	dsi->dsi = dw_mipi_dsi_probe(pdev, &dsi->pdata);
 	if (IS_ERR(dsi->dsi)) {
 		ret = PTR_ERR(dsi->dsi);
 		dev_err_probe(dev, ret, "Failed to initialize mipi dsi host\n");
 		goto err_dsi_probe;
 	}
 
+	/*
+	 * We need to wait for the generic bridge to probe before enabling and
+	 * register the internal pixel clock.
+	 */
+	ret = clk_prepare_enable(dsi->pclk);
+	if (ret) {
+		DRM_ERROR("%s: Failed to enable peripheral clk\n", __func__);
+		goto err_dsi_probe;
+	}
+
+	ret = dw_mipi_dsi_clk_register(dsi, dev);
+	if (ret) {
+		DRM_ERROR("Failed to register DSI pixel clock: %d\n", ret);
+		clk_disable_unprepare(dsi->pclk);
+		goto err_dsi_probe;
+	}
+
+	clk_disable_unprepare(dsi->pclk);
+
 	return 0;
 
 err_dsi_probe:
@@ -542,12 +726,13 @@ static void dw_mipi_dsi_stm_remove(struct platform_device *pdev)
 
 	dw_mipi_dsi_remove(dsi->dsi);
 	clk_disable_unprepare(dsi->pllref_clk);
+	dw_mipi_dsi_clk_unregister(dsi);
 	regulator_disable(dsi->vdd_supply);
 }
 
 static int dw_mipi_dsi_stm_suspend(struct device *dev)
 {
-	struct dw_mipi_dsi_stm *dsi = dw_mipi_dsi_stm_plat_data.priv_data;
+	struct dw_mipi_dsi_stm *dsi = dev_get_drvdata(dev);
 
 	DRM_DEBUG_DRIVER("\n");
 
@@ -560,7 +745,7 @@ static int dw_mipi_dsi_stm_suspend(struct device *dev)
 
 static int dw_mipi_dsi_stm_resume(struct device *dev)
 {
-	struct dw_mipi_dsi_stm *dsi = dw_mipi_dsi_stm_plat_data.priv_data;
+	struct dw_mipi_dsi_stm *dsi = dev_get_drvdata(dev);
 	int ret;
 
 	DRM_DEBUG_DRIVER("\n");
-- 
2.25.1


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

Re: [PATCH RESEND v3 1/3] drm/stm: dsi: use new SYSTEM_SLEEP_PM_OPS() macro

[Yannick FERTRE]

Hi Raphael,

thanks for the patch.

Acked-by: Yannick Fertre <yannick.fertre@foss.st.com>

Tested-by: Yannick Fertre <yannick.fertre@foss.st.com>

BR


Le 29/01/2024 à 11:41, Raphael Gallais-Pou a écrit :
> Use RUNTIME_PM_OPS() instead of the old SET_SYSTEM_SLEEP_PM_OPS().
> This means we don't need  __maybe_unused on the functions.
>
> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
> ---
>   drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> index d5f8c923d7bc..b1aee43d51e9 100644
> --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> @@ -544,7 +544,7 @@ static void dw_mipi_dsi_stm_remove(struct platform_device *pdev)
>   	regulator_disable(dsi->vdd_supply);
>   }
>   
> -static int __maybe_unused dw_mipi_dsi_stm_suspend(struct device *dev)
> +static int dw_mipi_dsi_stm_suspend(struct device *dev)
>   {
>   	struct dw_mipi_dsi_stm *dsi = dw_mipi_dsi_stm_plat_data.priv_data;
>   
> @@ -556,7 +556,7 @@ static int __maybe_unused dw_mipi_dsi_stm_suspend(struct device *dev)
>   	return 0;
>   }
>   
> -static int __maybe_unused dw_mipi_dsi_stm_resume(struct device *dev)
> +static int dw_mipi_dsi_stm_resume(struct device *dev)
>   {
>   	struct dw_mipi_dsi_stm *dsi = dw_mipi_dsi_stm_plat_data.priv_data;
>   	int ret;
> @@ -580,8 +580,8 @@ static int __maybe_unused dw_mipi_dsi_stm_resume(struct device *dev)
>   }
>   
>   static const struct dev_pm_ops dw_mipi_dsi_stm_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(dw_mipi_dsi_stm_suspend,
> -				dw_mipi_dsi_stm_resume)
> +	SYSTEM_SLEEP_PM_OPS(dw_mipi_dsi_stm_suspend,
> +			    dw_mipi_dsi_stm_resume)
>   };
>   
>   static struct platform_driver dw_mipi_dsi_stm_driver = {

Re: [PATCH RESEND v3 2/3] drm/stm: dsi: add pm runtime ops

[Yannick FERTRE]

Hi Raphael,

thanks for the patch.

Acked-by: Yannick Fertre <yannick.fertre@foss.st.com>

Tested-by: Yannick Fertre <yannick.fertre@foss.st.com>

BR


Le 29/01/2024 à 11:41, Raphael Gallais-Pou a écrit :
> From: Yannick Fertre <yannick.fertre@foss.st.com>
>
> Update control of clocks and supply thanks to the PM runtime
> mechanism to avoid kernel crash during a system suspend.
>
> Signed-off-by: Yannick Fertre <yannick.fertre@foss.st.com>
> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
> ---
> Changes in v2:
> 	- Changed SET_RUNTIME_PM_OPS to RUNTIME_PM_OPS and removed
> 	__maybe_unused
> ---
>   drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 24 ++++++++++++++++++------
>   1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> index b1aee43d51e9..82fff9e84345 100644
> --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> @@ -11,6 +11,7 @@
>   #include <linux/mod_devicetable.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>   #include <linux/regulator/consumer.h>
>   
>   #include <video/mipi_display.h>
> @@ -77,6 +78,7 @@ enum dsi_color {
>   struct dw_mipi_dsi_stm {
>   	void __iomem *base;
>   	struct clk *pllref_clk;
> +	struct clk *pclk;
>   	struct dw_mipi_dsi *dsi;
>   	u32 hw_version;
>   	int lane_min_kbps;
> @@ -443,7 +445,6 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct dw_mipi_dsi_stm *dsi;
> -	struct clk *pclk;
>   	int ret;
>   
>   	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> @@ -483,21 +484,21 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
>   		goto err_clk_get;
>   	}
>   
> -	pclk = devm_clk_get(dev, "pclk");
> -	if (IS_ERR(pclk)) {
> -		ret = PTR_ERR(pclk);
> +	dsi->pclk = devm_clk_get(dev, "pclk");
> +	if (IS_ERR(dsi->pclk)) {
> +		ret = PTR_ERR(dsi->pclk);
>   		DRM_ERROR("Unable to get peripheral clock: %d\n", ret);
>   		goto err_dsi_probe;
>   	}
>   
> -	ret = clk_prepare_enable(pclk);
> +	ret = clk_prepare_enable(dsi->pclk);
>   	if (ret) {
>   		DRM_ERROR("%s: Failed to enable peripheral clk\n", __func__);
>   		goto err_dsi_probe;
>   	}
>   
>   	dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION;
> -	clk_disable_unprepare(pclk);
> +	clk_disable_unprepare(dsi->pclk);
>   
>   	if (dsi->hw_version != HWVER_130 && dsi->hw_version != HWVER_131) {
>   		ret = -ENODEV;
> @@ -551,6 +552,7 @@ static int dw_mipi_dsi_stm_suspend(struct device *dev)
>   	DRM_DEBUG_DRIVER("\n");
>   
>   	clk_disable_unprepare(dsi->pllref_clk);
> +	clk_disable_unprepare(dsi->pclk);
>   	regulator_disable(dsi->vdd_supply);
>   
>   	return 0;
> @@ -569,8 +571,16 @@ static int dw_mipi_dsi_stm_resume(struct device *dev)
>   		return ret;
>   	}
>   
> +	ret = clk_prepare_enable(dsi->pclk);
> +	if (ret) {
> +		regulator_disable(dsi->vdd_supply);
> +		DRM_ERROR("Failed to enable pclk: %d\n", ret);
> +		return ret;
> +	}
> +
>   	ret = clk_prepare_enable(dsi->pllref_clk);
>   	if (ret) {
> +		clk_disable_unprepare(dsi->pclk);
>   		regulator_disable(dsi->vdd_supply);
>   		DRM_ERROR("Failed to enable pllref_clk: %d\n", ret);
>   		return ret;
> @@ -582,6 +592,8 @@ static int dw_mipi_dsi_stm_resume(struct device *dev)
>   static const struct dev_pm_ops dw_mipi_dsi_stm_pm_ops = {
>   	SYSTEM_SLEEP_PM_OPS(dw_mipi_dsi_stm_suspend,
>   			    dw_mipi_dsi_stm_resume)
> +	RUNTIME_PM_OPS(dw_mipi_dsi_stm_suspend,
> +		       dw_mipi_dsi_stm_resume, NULL)
>   };
>   
>   static struct platform_driver dw_mipi_dsi_stm_driver = {

Re: [PATCH RESEND v3 3/3] drm/stm: dsi: expose DSI PHY internal clock

[Yannick FERTRE]

Hi Raphael,

thanks for the patch.

Acked-by: Yannick Fertre <yannick.fertre@foss.st.com>

Tested-by: Yannick Fertre <yannick.fertre@foss.st.com>

BR


Le 29/01/2024 à 11:41, Raphael Gallais-Pou a écrit :
> 	DSISRC __________
> 	               __\_
> 	              |    \
> 	pll4_p_ck   ->|  1  |____dsi_k
> 	ck_dsi_phy  ->|  0  |
> 	              |____/
>
> A DSI clock is missing in the clock framework. Looking at the
> clk_summary, it appears that 'ck_dsi_phy' is not implemented. Since the
> DSI kernel clock is based on the internal DSI pll. The common clock
> driver can not directly expose this 'ck_dsi_phy' clock because it does
> not contain any common registers with the DSI. Thus it needs to be done
> directly within the DSI phy driver.
>
> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
> ---
> Changes in v3:
> 	- Fix smatch warning:
> 	.../dw_mipi_dsi-stm.c:719 dw_mipi_dsi_stm_probe() warn: 'dsi->pclk'
> 	from clk_prepare_enable() not released on lines: 719.
> ---
>   drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 247 ++++++++++++++++++++++----
>   1 file changed, 216 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> index 82fff9e84345..b20123854c4a 100644
> --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> @@ -7,7 +7,9 @@
>    */
>   
>   #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>   #include <linux/iopoll.h>
> +#include <linux/kernel.h>
>   #include <linux/mod_devicetable.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
> @@ -77,9 +79,12 @@ enum dsi_color {
>   
>   struct dw_mipi_dsi_stm {
>   	void __iomem *base;
> +	struct device *dev;
>   	struct clk *pllref_clk;
>   	struct clk *pclk;
> +	struct clk_hw txbyte_clk;
>   	struct dw_mipi_dsi *dsi;
> +	struct dw_mipi_dsi_plat_data pdata;
>   	u32 hw_version;
>   	int lane_min_kbps;
>   	int lane_max_kbps;
> @@ -196,29 +201,198 @@ static int dsi_pll_get_params(struct dw_mipi_dsi_stm *dsi,
>   	return 0;
>   }
>   
> -static int dw_mipi_dsi_phy_init(void *priv_data)
> +#define clk_to_dw_mipi_dsi_stm(clk) \
> +	container_of(clk, struct dw_mipi_dsi_stm, txbyte_clk)
> +
> +static void dw_mipi_dsi_clk_disable(struct clk_hw *clk)
>   {
> -	struct dw_mipi_dsi_stm *dsi = priv_data;
> +	struct dw_mipi_dsi_stm *dsi = clk_to_dw_mipi_dsi_stm(clk);
> +
> +	DRM_DEBUG_DRIVER("\n");
> +
> +	/* Disable the DSI PLL */
> +	dsi_clear(dsi, DSI_WRPCR, WRPCR_PLLEN);
> +
> +	/* Disable the regulator */
> +	dsi_clear(dsi, DSI_WRPCR, WRPCR_REGEN | WRPCR_BGREN);
> +}
> +
> +static int dw_mipi_dsi_clk_enable(struct clk_hw *clk)
> +{
> +	struct dw_mipi_dsi_stm *dsi = clk_to_dw_mipi_dsi_stm(clk);
>   	u32 val;
>   	int ret;
>   
> +	DRM_DEBUG_DRIVER("\n");
> +
>   	/* Enable the regulator */
>   	dsi_set(dsi, DSI_WRPCR, WRPCR_REGEN | WRPCR_BGREN);
> -	ret = readl_poll_timeout(dsi->base + DSI_WISR, val, val & WISR_RRS,
> -				 SLEEP_US, TIMEOUT_US);
> +	ret = readl_poll_timeout_atomic(dsi->base + DSI_WISR, val, val & WISR_RRS,
> +					SLEEP_US, TIMEOUT_US);
>   	if (ret)
>   		DRM_DEBUG_DRIVER("!TIMEOUT! waiting REGU, let's continue\n");
>   
>   	/* Enable the DSI PLL & wait for its lock */
>   	dsi_set(dsi, DSI_WRPCR, WRPCR_PLLEN);
> -	ret = readl_poll_timeout(dsi->base + DSI_WISR, val, val & WISR_PLLLS,
> -				 SLEEP_US, TIMEOUT_US);
> +	ret = readl_poll_timeout_atomic(dsi->base + DSI_WISR, val, val & WISR_PLLLS,
> +					SLEEP_US, TIMEOUT_US);
>   	if (ret)
>   		DRM_DEBUG_DRIVER("!TIMEOUT! waiting PLL, let's continue\n");
>   
>   	return 0;
>   }
>   
> +static int dw_mipi_dsi_clk_is_enabled(struct clk_hw *hw)
> +{
> +	struct dw_mipi_dsi_stm *dsi = clk_to_dw_mipi_dsi_stm(hw);
> +
> +	return dsi_read(dsi, DSI_WRPCR) & WRPCR_PLLEN;
> +}
> +
> +static unsigned long dw_mipi_dsi_clk_recalc_rate(struct clk_hw *hw,
> +						 unsigned long parent_rate)
> +{
> +	struct dw_mipi_dsi_stm *dsi = clk_to_dw_mipi_dsi_stm(hw);
> +	unsigned int idf, ndiv, odf, pll_in_khz, pll_out_khz;
> +	u32 val;
> +
> +	DRM_DEBUG_DRIVER("\n");
> +
> +	pll_in_khz = (unsigned int)(parent_rate / 1000);
> +
> +	val = dsi_read(dsi, DSI_WRPCR);
> +
> +	idf = (val & WRPCR_IDF) >> 11;
> +	if (!idf)
> +		idf = 1;
> +	ndiv = (val & WRPCR_NDIV) >> 2;
> +	odf = int_pow(2, (val & WRPCR_ODF) >> 16);
> +
> +	/* Get the adjusted pll out value */
> +	pll_out_khz = dsi_pll_get_clkout_khz(pll_in_khz, idf, ndiv, odf);
> +
> +	return (unsigned long)pll_out_khz * 1000;
> +}
> +
> +static long dw_mipi_dsi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> +				       unsigned long *parent_rate)
> +{
> +	struct dw_mipi_dsi_stm *dsi = clk_to_dw_mipi_dsi_stm(hw);
> +	unsigned int idf, ndiv, odf, pll_in_khz, pll_out_khz;
> +	int ret;
> +
> +	DRM_DEBUG_DRIVER("\n");
> +
> +	pll_in_khz = (unsigned int)(*parent_rate / 1000);
> +
> +	/* Compute best pll parameters */
> +	idf = 0;
> +	ndiv = 0;
> +	odf = 0;
> +
> +	ret = dsi_pll_get_params(dsi, pll_in_khz, rate / 1000,
> +				 &idf, &ndiv, &odf);
> +	if (ret)
> +		DRM_WARN("Warning dsi_pll_get_params(): bad params\n");
> +
> +	/* Get the adjusted pll out value */
> +	pll_out_khz = dsi_pll_get_clkout_khz(pll_in_khz, idf, ndiv, odf);
> +
> +	return pll_out_khz * 1000;
> +}
> +
> +static int dw_mipi_dsi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +				    unsigned long parent_rate)
> +{
> +	struct dw_mipi_dsi_stm *dsi = clk_to_dw_mipi_dsi_stm(hw);
> +	unsigned int idf, ndiv, odf, pll_in_khz, pll_out_khz;
> +	int ret;
> +	u32 val;
> +
> +	DRM_DEBUG_DRIVER("\n");
> +
> +	pll_in_khz = (unsigned int)(parent_rate / 1000);
> +
> +	/* Compute best pll parameters */
> +	idf = 0;
> +	ndiv = 0;
> +	odf = 0;
> +
> +	ret = dsi_pll_get_params(dsi, pll_in_khz, rate / 1000, &idf, &ndiv, &odf);
> +	if (ret)
> +		DRM_WARN("Warning dsi_pll_get_params(): bad params\n");
> +
> +	/* Get the adjusted pll out value */
> +	pll_out_khz = dsi_pll_get_clkout_khz(pll_in_khz, idf, ndiv, odf);
> +
> +	/* Set the PLL division factors */
> +	dsi_update_bits(dsi, DSI_WRPCR,	WRPCR_NDIV | WRPCR_IDF | WRPCR_ODF,
> +			(ndiv << 2) | (idf << 11) | ((ffs(odf) - 1) << 16));
> +
> +	/* Compute uix4 & set the bit period in high-speed mode */
> +	val = 4000000 / pll_out_khz;
> +	dsi_update_bits(dsi, DSI_WPCR0, WPCR0_UIX4, val);
> +
> +	return 0;
> +}
> +
> +static void dw_mipi_dsi_clk_unregister(void *data)
> +{
> +	struct dw_mipi_dsi_stm *dsi = data;
> +
> +	DRM_DEBUG_DRIVER("\n");
> +
> +	of_clk_del_provider(dsi->dev->of_node);
> +	clk_hw_unregister(&dsi->txbyte_clk);
> +}
> +
> +static const struct clk_ops dw_mipi_dsi_stm_clk_ops = {
> +	.enable = dw_mipi_dsi_clk_enable,
> +	.disable = dw_mipi_dsi_clk_disable,
> +	.is_enabled = dw_mipi_dsi_clk_is_enabled,
> +	.recalc_rate = dw_mipi_dsi_clk_recalc_rate,
> +	.round_rate = dw_mipi_dsi_clk_round_rate,
> +	.set_rate = dw_mipi_dsi_clk_set_rate,
> +};
> +
> +static struct clk_init_data cdata_init = {
> +	.name = "ck_dsi_phy",
> +	.ops = &dw_mipi_dsi_stm_clk_ops,
> +	.parent_names = (const char * []) {"ck_hse"},
> +	.num_parents = 1,
> +};
> +
> +static int dw_mipi_dsi_clk_register(struct dw_mipi_dsi_stm *dsi,
> +				    struct device *dev)
> +{
> +	struct device_node *node = dev->of_node;
> +	int ret;
> +
> +	DRM_DEBUG_DRIVER("Registering clk\n");
> +
> +	dsi->txbyte_clk.init = &cdata_init;
> +
> +	ret = clk_hw_register(dev, &dsi->txbyte_clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_clk_add_hw_provider(node, of_clk_hw_simple_get,
> +				     &dsi->txbyte_clk);
> +	if (ret)
> +		clk_hw_unregister(&dsi->txbyte_clk);
> +
> +	return ret;
> +}
> +
> +static int dw_mipi_dsi_phy_init(void *priv_data)
> +{
> +	struct dw_mipi_dsi_stm *dsi = priv_data;
> +	int ret;
> +
> +	ret = clk_prepare_enable(dsi->txbyte_clk.clk);
> +	return ret;
> +}
> +
>   static void dw_mipi_dsi_phy_power_on(void *priv_data)
>   {
>   	struct dw_mipi_dsi_stm *dsi = priv_data;
> @@ -235,6 +409,8 @@ static void dw_mipi_dsi_phy_power_off(void *priv_data)
>   
>   	DRM_DEBUG_DRIVER("\n");
>   
> +	clk_disable_unprepare(dsi->txbyte_clk.clk);
> +
>   	/* Disable the DSI wrapper */
>   	dsi_clear(dsi, DSI_WCR, WCR_DSIEN);
>   }
> @@ -245,9 +421,8 @@ dw_mipi_dsi_get_lane_mbps(void *priv_data, const struct drm_display_mode *mode,
>   			  unsigned int *lane_mbps)
>   {
>   	struct dw_mipi_dsi_stm *dsi = priv_data;
> -	unsigned int idf, ndiv, odf, pll_in_khz, pll_out_khz;
> +	unsigned int pll_in_khz, pll_out_khz;
>   	int ret, bpp;
> -	u32 val;
>   
>   	pll_in_khz = (unsigned int)(clk_get_rate(dsi->pllref_clk) / 1000);
>   
> @@ -268,25 +443,10 @@ dw_mipi_dsi_get_lane_mbps(void *priv_data, const struct drm_display_mode *mode,
>   		DRM_WARN("Warning min phy mbps is used\n");
>   	}
>   
> -	/* Compute best pll parameters */
> -	idf = 0;
> -	ndiv = 0;
> -	odf = 0;
> -	ret = dsi_pll_get_params(dsi, pll_in_khz, pll_out_khz,
> -				 &idf, &ndiv, &odf);
> +	ret = clk_set_rate((dsi->txbyte_clk.clk), pll_out_khz * 1000);
>   	if (ret)
> -		DRM_WARN("Warning dsi_pll_get_params(): bad params\n");
> -
> -	/* Get the adjusted pll out value */
> -	pll_out_khz = dsi_pll_get_clkout_khz(pll_in_khz, idf, ndiv, odf);
> -
> -	/* Set the PLL division factors */
> -	dsi_update_bits(dsi, DSI_WRPCR,	WRPCR_NDIV | WRPCR_IDF | WRPCR_ODF,
> -			(ndiv << 2) | (idf << 11) | ((ffs(odf) - 1) << 16));
> -
> -	/* Compute uix4 & set the bit period in high-speed mode */
> -	val = 4000000 / pll_out_khz;
> -	dsi_update_bits(dsi, DSI_WPCR0, WPCR0_UIX4, val);
> +		DRM_DEBUG_DRIVER("ERROR Could not set rate of %d to %s clk->name",
> +				 pll_out_khz, clk_hw_get_name(&dsi->txbyte_clk));
>   
>   	/* Select video mode by resetting DSIM bit */
>   	dsi_clear(dsi, DSI_WCFGR, WCFGR_DSIM);
> @@ -445,6 +605,7 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct dw_mipi_dsi_stm *dsi;
> +	const struct dw_mipi_dsi_plat_data *pdata = of_device_get_match_data(dev);
>   	int ret;
>   
>   	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> @@ -514,18 +675,41 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
>   		dsi->lane_max_kbps *= 2;
>   	}
>   
> -	dw_mipi_dsi_stm_plat_data.base = dsi->base;
> -	dw_mipi_dsi_stm_plat_data.priv_data = dsi;
> +	dsi->pdata = *pdata;
> +	dsi->pdata.base = dsi->base;
> +	dsi->pdata.priv_data = dsi;
> +
> +	dsi->pdata.max_data_lanes = 2;
> +	dsi->pdata.phy_ops = &dw_mipi_dsi_stm_phy_ops;
>   
>   	platform_set_drvdata(pdev, dsi);
>   
> -	dsi->dsi = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
> +	dsi->dsi = dw_mipi_dsi_probe(pdev, &dsi->pdata);
>   	if (IS_ERR(dsi->dsi)) {
>   		ret = PTR_ERR(dsi->dsi);
>   		dev_err_probe(dev, ret, "Failed to initialize mipi dsi host\n");
>   		goto err_dsi_probe;
>   	}
>   
> +	/*
> +	 * We need to wait for the generic bridge to probe before enabling and
> +	 * register the internal pixel clock.
> +	 */
> +	ret = clk_prepare_enable(dsi->pclk);
> +	if (ret) {
> +		DRM_ERROR("%s: Failed to enable peripheral clk\n", __func__);
> +		goto err_dsi_probe;
> +	}
> +
> +	ret = dw_mipi_dsi_clk_register(dsi, dev);
> +	if (ret) {
> +		DRM_ERROR("Failed to register DSI pixel clock: %d\n", ret);
> +		clk_disable_unprepare(dsi->pclk);
> +		goto err_dsi_probe;
> +	}
> +
> +	clk_disable_unprepare(dsi->pclk);
> +
>   	return 0;
>   
>   err_dsi_probe:
> @@ -542,12 +726,13 @@ static void dw_mipi_dsi_stm_remove(struct platform_device *pdev)
>   
>   	dw_mipi_dsi_remove(dsi->dsi);
>   	clk_disable_unprepare(dsi->pllref_clk);
> +	dw_mipi_dsi_clk_unregister(dsi);
>   	regulator_disable(dsi->vdd_supply);
>   }
>   
>   static int dw_mipi_dsi_stm_suspend(struct device *dev)
>   {
> -	struct dw_mipi_dsi_stm *dsi = dw_mipi_dsi_stm_plat_data.priv_data;
> +	struct dw_mipi_dsi_stm *dsi = dev_get_drvdata(dev);
>   
>   	DRM_DEBUG_DRIVER("\n");
>   
> @@ -560,7 +745,7 @@ static int dw_mipi_dsi_stm_suspend(struct device *dev)
>   
>   static int dw_mipi_dsi_stm_resume(struct device *dev)
>   {
> -	struct dw_mipi_dsi_stm *dsi = dw_mipi_dsi_stm_plat_data.priv_data;
> +	struct dw_mipi_dsi_stm *dsi = dev_get_drvdata(dev);
>   	int ret;
>   
>   	DRM_DEBUG_DRIVER("\n");

Re: [PATCH RESEND v3 0/3] Update STM DSI PHY driver

[Philippe CORNU]

On 1/29/24 11:41, Raphael Gallais-Pou wrote:
> 
> This patch series aims to add several features of the dw-mipi-dsi phy
> driver that are missing or need to be updated.
> 
> First patch update a PM macro.
> 
> Second patch adds runtime PM functionality to the driver.
> 
> Third patch adds a clock provider generated by the PHY itself.  As
> explained in the commit log of the second patch, a clock declaration is
> missing.  Since this clock is parent of 'dsi_k', it leads to an orphan
> clock.  Most importantly this patch is an anticipation for future
> versions of the DSI PHY, and its inclusion within the display subsystem
> and the DRM framework.
> 
> Last patch fixes a corner effect introduced previously.  Since 'dsi' and
> 'dsi_k' are gated by the same bit on the same register, both reference
> work as peripheral clock in the device-tree.
> 
> ---
> Changes in v3-resend:
> 	- Removed last patch as it has been merged
> https://lore.kernel.org/lkml/bf49f4c9-9e81-4c91-972d-13782d996aaa@foss.st.com/
> 
> Changes in v3:
> 	- Fix smatch warning (disable dsi->pclk when clk_register fails)
> 
> Changes in v2:
> 	- Added patch 1/4 to use SYSTEM_SLEEP_PM_OPS instead of old macro
> 	  and removed __maybe_used for accordingly
> 	- Changed SET_RUNTIME_PM_OPS to RUNTIME_PM_OPS
> 
> Raphael Gallais-Pou (3):
>    drm/stm: dsi: use new SYSTEM_SLEEP_PM_OPS() macro
>    drm/stm: dsi: expose DSI PHY internal clock
> 
> Yannick Fertre (1):
>    drm/stm: dsi: add pm runtime ops
> 
>   drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 279 ++++++++++++++++++++++----
>   1 file changed, 238 insertions(+), 41 deletions(-)
> 

Hi Raphaël & Yannick,
Applied on drm-misc-next.
Many thanks,
Philippe :-)

Re: [Linux-stm32] [PATCH RESEND v3 0/3] Update STM DSI PHY driver

[Yanjun Yang]

On Fri, Jun 28, 2024 at 8:47 PM Philippe CORNU
<philippe.cornu@foss.st.com> wrote:
>
>
>
> On 1/29/24 11:41, Raphael Gallais-Pou wrote:
> >
> > This patch series aims to add several features of the dw-mipi-dsi phy
> > driver that are missing or need to be updated.
> >
> > First patch update a PM macro.
> >
> > Second patch adds runtime PM functionality to the driver.
> >
> > Third patch adds a clock provider generated by the PHY itself.  As
> > explained in the commit log of the second patch, a clock declaration is
> > missing.  Since this clock is parent of 'dsi_k', it leads to an orphan
> > clock.  Most importantly this patch is an anticipation for future
> > versions of the DSI PHY, and its inclusion within the display subsystem
> > and the DRM framework.
> >
> > Last patch fixes a corner effect introduced previously.  Since 'dsi' and
> > 'dsi_k' are gated by the same bit on the same register, both reference
> > work as peripheral clock in the device-tree.
> >

This patch (commit id:185f99b614427360) seems to break the dsi of
stm32f469 chip.
I'm not familiar with the drm and the clock framework, maybe it's
because there is no
 "ck_dsi_phy" defined for stm32f469.
PS:  Sorry for receiving multiple copies of this email, I forgot to
use plain text mode last time.

> > ---
> > Changes in v3-resend:
> >       - Removed last patch as it has been merged
> > https://lore.kernel.org/lkml/bf49f4c9-9e81-4c91-972d-13782d996aaa@foss.st.com/
> >
> > Changes in v3:
> >       - Fix smatch warning (disable dsi->pclk when clk_register fails)
> >
> > Changes in v2:
> >       - Added patch 1/4 to use SYSTEM_SLEEP_PM_OPS instead of old macro
> >         and removed __maybe_used for accordingly
> >       - Changed SET_RUNTIME_PM_OPS to RUNTIME_PM_OPS
> >
> > Raphael Gallais-Pou (3):
> >    drm/stm: dsi: use new SYSTEM_SLEEP_PM_OPS() macro
> >    drm/stm: dsi: expose DSI PHY internal clock
> >
> > Yannick Fertre (1):
> >    drm/stm: dsi: add pm runtime ops
> >
> >   drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 279 ++++++++++++++++++++++----
> >   1 file changed, 238 insertions(+), 41 deletions(-)
> >
>
> Hi Raphaël & Yannick,
> Applied on drm-misc-next.
> Many thanks,
> Philippe :-)
> _______________________________________________
> Linux-stm32 mailing list
> Linux-stm32@st-md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32

Re: [Linux-stm32] [PATCH RESEND v3 0/3] Update STM DSI PHY driver

[Philippe CORNU]

On 7/22/24 10:38, Yanjun Yang wrote:
> On Fri, Jun 28, 2024 at 8:47 PM Philippe CORNU
> <philippe.cornu@foss.st.com> wrote:
>>
>>
>>
>> On 1/29/24 11:41, Raphael Gallais-Pou wrote:
>>>
>>> This patch series aims to add several features of the dw-mipi-dsi phy
>>> driver that are missing or need to be updated.
>>>
>>> First patch update a PM macro.
>>>
>>> Second patch adds runtime PM functionality to the driver.
>>>
>>> Third patch adds a clock provider generated by the PHY itself.  As
>>> explained in the commit log of the second patch, a clock declaration is
>>> missing.  Since this clock is parent of 'dsi_k', it leads to an orphan
>>> clock.  Most importantly this patch is an anticipation for future
>>> versions of the DSI PHY, and its inclusion within the display subsystem
>>> and the DRM framework.
>>>
>>> Last patch fixes a corner effect introduced previously.  Since 'dsi' and
>>> 'dsi_k' are gated by the same bit on the same register, both reference
>>> work as peripheral clock in the device-tree.
>>>
> 
> This patch (commit id:185f99b614427360) seems to break the dsi of
> stm32f469 chip.
> I'm not familiar with the drm and the clock framework, maybe it's
> because there is no
>   "ck_dsi_phy" defined for stm32f469.
> PS:  Sorry for receiving multiple copies of this email, I forgot to
> use plain text mode last time.
> 

Hi,
Thank you for letting us know that there was this error. We should have 
detected this before merging, really sorry for the problems caused by 
this patch. We will investigate the issue and get back to you as soon as 
possible. In the meantime, I think you can revert this patch in your git 
tree.

Philippe :-)

>>> ---
>>> Changes in v3-resend:
>>>        - Removed last patch as it has been merged
>>> https://lore.kernel.org/lkml/bf49f4c9-9e81-4c91-972d-13782d996aaa@foss.st.com/
>>>
>>> Changes in v3:
>>>        - Fix smatch warning (disable dsi->pclk when clk_register fails)
>>>
>>> Changes in v2:
>>>        - Added patch 1/4 to use SYSTEM_SLEEP_PM_OPS instead of old macro
>>>          and removed __maybe_used for accordingly
>>>        - Changed SET_RUNTIME_PM_OPS to RUNTIME_PM_OPS
>>>
>>> Raphael Gallais-Pou (3):
>>>     drm/stm: dsi: use new SYSTEM_SLEEP_PM_OPS() macro
>>>     drm/stm: dsi: expose DSI PHY internal clock
>>>
>>> Yannick Fertre (1):
>>>     drm/stm: dsi: add pm runtime ops
>>>
>>>    drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 279 ++++++++++++++++++++++----
>>>    1 file changed, 238 insertions(+), 41 deletions(-)
>>>
>>
>> Hi Raphaël & Yannick,
>> Applied on drm-misc-next.
>> Many thanks,
>> Philippe :-)
>> _______________________________________________
>> Linux-stm32 mailing list
>> Linux-stm32@st-md-mailman.stormreply.com
>> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32

Re: [Linux-stm32] [PATCH RESEND v3 0/3] Update STM DSI PHY driver

[Yanjun Yang]

On Fri, Jul 26, 2024 at 09:55:35AM +0200, Philippe CORNU wrote:
> 
> 
> On 7/22/24 10:38, Yanjun Yang wrote:
> > 
> > This patch (commit id:185f99b614427360) seems to break the dsi of
> > stm32f469 chip.
> > I'm not familiar with the drm and the clock framework, maybe it's
> > because there is no
> >   "ck_dsi_phy" defined for stm32f469.
> > PS:  Sorry for receiving multiple copies of this email, I forgot to
> > use plain text mode last time.
> > 
> 
> Hi,
> Thank you for letting us know that there was this error. We should have
> detected this before merging, really sorry for the problems caused by this
> patch. We will investigate the issue and get back to you as soon as
> possible. In the meantime, I think you can revert this patch in your git
> tree.
> 
> Philippe :-)
> 

Hi,
After some testing, the reason behind my problem is the parent's name of
'clk_dsi_phy' for stm32f4 is 'clk-hse' other than 'ck_hse'.  I don't
know which is the better why to fix it:
1. Change "ck_hse" to "clk-hse" in where "clk_dsi_phy" is defined.
2. Use "pll_in_khz = clk_get_rate(dsi->pllref_clk) / 1000" instead of
   "pll_in_khz = (unsigned int)(parent_rate / 1000)" when get the clock
   rate.

Both method can fix my problem. The first one might break other
platforms. Maybe I should change the clock name of 'clk-hse'. However,
I can't find the defination of this clock name for stm32f4.

Re: [Linux-stm32] [PATCH RESEND v3 0/3] Update STM DSI PHY driver

[Raphaël Gallais-Pou]

Le 29/07/2024 à 15:28, Yanjun Yang a écrit :
> On Fri, Jul 26, 2024 at 09:55:35AM +0200, Philippe CORNU wrote:
>>
>>
>> On 7/22/24 10:38, Yanjun Yang wrote:
>>>
>>> This patch (commit id:185f99b614427360) seems to break the dsi of
>>> stm32f469 chip.
>>> I'm not familiar with the drm and the clock framework, maybe it's
>>> because there is no
>>>    "ck_dsi_phy" defined for stm32f469.
>>> PS:  Sorry for receiving multiple copies of this email, I forgot to
>>> use plain text mode last time.
>>>
>>
>> Hi,
>> Thank you for letting us know that there was this error. We should have
>> detected this before merging, really sorry for the problems caused by this
>> patch. We will investigate the issue and get back to you as soon as
>> possible. In the meantime, I think you can revert this patch in your git
>> tree.
>>
>> Philippe :-)
>>
> 
> Hi,
Hi,

FYI
DSI clock tree for stm32f469 can be found here:
https://www.st.com/resource/en/reference_manual/rm0386-stm32f469xx-and-stm32f479xx-advanced-armbased-32bit-mcus-stmicroelectronics.pdf

Refer to Figure 17: DSI clock tree.

After some research I think "ck_dsi_phy" was introduced in stm32h7 
platforms. There is a mux which interfaces between various clocks (among 
ck_hse) and the byte lane clock. stm32f469 has a much simpler clock tree 
in which we did not bother to implement this "go-between" clock, even 
though they is an equivalent of the mux.

> After some testing, the reason behind my problem is the parent's name of
> 'clk_dsi_phy' for stm32f4 is 'clk-hse' other than 'ck_hse'.  I don't
> know which is the better why to fix it:
> 1. Change "ck_hse" to "clk-hse" in where "clk_dsi_phy" is defined.
Doing so will definitely break other platforms.

> 2. Use "pll_in_khz = clk_get_rate(dsi->pllref_clk) / 1000" instead of
>     "pll_in_khz = (unsigned int)(parent_rate / 1000)" when get the clock
>     rate.
dsi->pllref_clk refers to the HSE clock if you take a look in the 
device-tree. This is the reason why this work on your setup. I doubt 
nevertheless that it wouldn't work on other platforms. But this would be 
a semantic nonsense, since the DSI byte lane clock is not always derived 
from HSE clock on other platforms.

Looking again at the clk-stm32f4 driver and the DSI clock tree linked, 
we can maybe implement the desired clock even if it is not represented 
on the diagram.

Eventually if this solution does not work we will go to the second 
solution you suggested and we will test it on all platforms.

@Philippe, @Yannick
Do you agree with this workflow ?

Regards,
Raphaël


> 
> Both method can fix my problem. The first one might break other
> platforms. Maybe I should change the clock name of 'clk-hse'. However,
> I can't find the defination of this clock name for stm32f4.

Re: [Linux-stm32] [PATCH RESEND v3 0/3] Update STM DSI PHY driver

[Yannick FERTRE]

Hi,

we don't give enough attention to older SOCs like stm32f469. This is an 
error on our part.

I think that to fix this point it would be necessary to define the clock 
hse as clock fix.

I hope to be able to release a patch before the end of August

Best regards

Yannick Fertré


Le 01/08/2024 à 11:07, Raphaël Gallais-Pou a écrit :
>
>
> Le 29/07/2024 à 15:28, Yanjun Yang a écrit :
>> On Fri, Jul 26, 2024 at 09:55:35AM +0200, Philippe CORNU wrote:
>>>
>>>
>>> On 7/22/24 10:38, Yanjun Yang wrote:
>>>>
>>>> This patch (commit id:185f99b614427360) seems to break the dsi of
>>>> stm32f469 chip.
>>>> I'm not familiar with the drm and the clock framework, maybe it's
>>>> because there is no
>>>>    "ck_dsi_phy" defined for stm32f469.
>>>> PS:  Sorry for receiving multiple copies of this email, I forgot to
>>>> use plain text mode last time.
>>>>
>>>
>>> Hi,
>>> Thank you for letting us know that there was this error. We should have
>>> detected this before merging, really sorry for the problems caused 
>>> by this
>>> patch. We will investigate the issue and get back to you as soon as
>>> possible. In the meantime, I think you can revert this patch in your 
>>> git
>>> tree.
>>>
>>> Philippe :-)
>>>
>>
>> Hi,
> Hi,
>
> FYI
> DSI clock tree for stm32f469 can be found here:
> https://www.st.com/resource/en/reference_manual/rm0386-stm32f469xx-and-stm32f479xx-advanced-armbased-32bit-mcus-stmicroelectronics.pdf 
>
>
> Refer to Figure 17: DSI clock tree.
>
> After some research I think "ck_dsi_phy" was introduced in stm32h7 
> platforms. There is a mux which interfaces between various clocks 
> (among ck_hse) and the byte lane clock. stm32f469 has a much simpler 
> clock tree in which we did not bother to implement this "go-between" 
> clock, even though they is an equivalent of the mux.
>
>> After some testing, the reason behind my problem is the parent's name of
>> 'clk_dsi_phy' for stm32f4 is 'clk-hse' other than 'ck_hse'.  I don't
>> know which is the better why to fix it:
>> 1. Change "ck_hse" to "clk-hse" in where "clk_dsi_phy" is defined.
> Doing so will definitely break other platforms.
>
>> 2. Use "pll_in_khz = clk_get_rate(dsi->pllref_clk) / 1000" instead of
>>     "pll_in_khz = (unsigned int)(parent_rate / 1000)" when get the clock
>>     rate.
> dsi->pllref_clk refers to the HSE clock if you take a look in the 
> device-tree. This is the reason why this work on your setup. I doubt 
> nevertheless that it wouldn't work on other platforms. But this would 
> be a semantic nonsense, since the DSI byte lane clock is not always 
> derived from HSE clock on other platforms.
>
> Looking again at the clk-stm32f4 driver and the DSI clock tree linked, 
> we can maybe implement the desired clock even if it is not represented 
> on the diagram.
>
> Eventually if this solution does not work we will go to the second 
> solution you suggested and we will test it on all platforms.
>
> @Philippe, @Yannick
> Do you agree with this workflow ?
>
> Regards,
> Raphaël
>
>
>>
>> Both method can fix my problem. The first one might break other
>> platforms. Maybe I should change the clock name of 'clk-hse'. However,
>> I can't find the defination of this clock name for stm32f4.