[PATCH V6 0/6] drm: bridge: samsung-dsim: Support variable clocking

[PATCH V6 0/6] drm: bridge: samsung-dsim: Support variable clocking

[Adam Ford]

This series fixes the blanking pack size and the PMS calculation.  It then
adds support to allows the DSIM to dynamically DPHY clocks, and support
non-burst mode while allowing the removal of the hard-coded clock values
for the PLL for imx8m mini/nano/plus, and it allows the removal of the
burst-clock device tree entry when burst-mode isn't supported by connected
devices like an HDMI brige.  In that event, the HS clock is set to the
value requested by the bridge chip.

This has been tested on both an i.MX8M Nano and i.MX8M Plus, and should
work on i.MX8M Mini as well. Marek Szyprowski has tested it on various
Exynos boards.

Adam Ford (5):
  drm: bridge: samsung-dsim: Fix PMS Calculator on imx8m[mnp]
  drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically
  drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY
  drm: bridge: samsung-dsim: Dynamically configure DPHY timing
  drm: bridge: samsung-dsim: Support non-burst mode

Lucas Stach (1):
  drm: bridge: samsung-dsim: fix blanking packet size calculation

 drivers/gpu/drm/bridge/Kconfig        |   1 +
 drivers/gpu/drm/bridge/samsung-dsim.c | 143 +++++++++++++++++++++-----
 include/drm/bridge/samsung-dsim.h     |   4 +
 3 files changed, 125 insertions(+), 23 deletions(-)


V6:  Squash-in an additional error fix from Lucas Stach regarding the
     DPHY calcuations.  Remove the dynamic_dphy variable and let
     everyone use the new calculations.  Move the hs_clock caching
     from patch 6 to patch 5 to go along with the DPHY calcuations
     since they are now based on the recorded hs_clock rate.
     
V5:  Update error message to dev_info and change them to indicate
     what is happening without sounding like an error when optional
     device tree entries are missing.

V4:  Undo some accidental whitespace changes, rename PS_TO_CYCLE
     variables to ps and hz from PS and MHz. Remove if check
     before the samsung_dsim_set_phy_ctrl call since it's
     unnecessary.
     Added additional tested-by and reviewed-by comments.
     Squash patches 6 and 7 together since the supporting
     non-burst (patch 6) mode doesn't really work until
     patch 7 was applied.

V3:  When checking if the bust-clock is present, only check for it
     in the device tree, and don't check the presence of the
     MIPI_DSI_MODE_VIDEO_BURST flag as it breaks an existing Exynos
     board.

     Add a new patch to the series to select GENERIC_PHY_MIPI_DPHY in
     Kconfig otherwise the build breaks on the 32-bit Exynos.

     Change vco_min variable name to min_freq

     Added tested-by from Chen-Yu Tsai

V2:  Instead of using my packet blanking calculation, this integrates
     on from Lucas Stach which gets modified later in the series to
     cache the value of the HS-clock instead of having to do the
     calucations again.

     Instead of completely eliminating the PLL clock frequency from
     the device tree, this makes it optional to avoid breaking some
     Samsung devices.  When the samsung,pll-clock-frequency is not
     found, it reads the value of the clock named "sclk_mipi"
     This also maintains backwards compatibility with older device
     trees.

     This also changes the DPHY calcuation from a Look-up table,
     a reverse engineered algorithm which uses
     phy_mipi_dphy_get_default_config to determine the standard
     nominal values and calculates the cycles necessary to update
     the DPHY timings accordingly.
     
-- 
2.39.2

[PATCH V6 1/6] drm: bridge: samsung-dsim: fix blanking packet size calculation

[Adam Ford]

From: Lucas Stach <l.stach@pengutronix.de>

Scale the blanking packet sizes to match the ratio between HS clock
and DPI interface clock. The controller seems to do internal scaling
to the number of active lanes, so we don't take those into account.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Adam Ford <aford173@gmail.com>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index e0a402a85787..2be3b58624c3 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -874,17 +874,29 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi)
 	u32 reg;
 
 	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
+		int byte_clk_khz = dsi->burst_clk_rate / 1000 / 8;
+		int hfp = (m->hsync_start - m->hdisplay) * byte_clk_khz / m->clock;
+		int hbp = (m->htotal - m->hsync_end) * byte_clk_khz / m->clock;
+		int hsa = (m->hsync_end - m->hsync_start) * byte_clk_khz / m->clock;
+
+		/* remove packet overhead when possible */
+		hfp = max(hfp - 6, 0);
+		hbp = max(hbp - 6, 0);
+		hsa = max(hsa - 6, 0);
+
+		dev_dbg(dsi->dev, "calculated hfp: %u, hbp: %u, hsa: %u",
+			hfp, hbp, hsa);
+
 		reg = DSIM_CMD_ALLOW(0xf)
 			| DSIM_STABLE_VFP(m->vsync_start - m->vdisplay)
 			| DSIM_MAIN_VBP(m->vtotal - m->vsync_end);
 		samsung_dsim_write(dsi, DSIM_MVPORCH_REG, reg);
 
-		reg = DSIM_MAIN_HFP(m->hsync_start - m->hdisplay)
-			| DSIM_MAIN_HBP(m->htotal - m->hsync_end);
+		reg = DSIM_MAIN_HFP(hfp) | DSIM_MAIN_HBP(hbp);
 		samsung_dsim_write(dsi, DSIM_MHPORCH_REG, reg);
 
 		reg = DSIM_MAIN_VSA(m->vsync_end - m->vsync_start)
-			| DSIM_MAIN_HSA(m->hsync_end - m->hsync_start);
+			| DSIM_MAIN_HSA(hsa);
 		samsung_dsim_write(dsi, DSIM_MSYNC_REG, reg);
 	}
 	reg =  DSIM_MAIN_HRESOL(m->hdisplay, num_bits_resol) |
-- 
2.39.2

[PATCH V6 2/6] drm: bridge: samsung-dsim: Fix PMS Calculator on imx8m[mnp]

[Adam Ford]

According to Table 13-45 of the i.MX8M Mini Reference Manual, the min
and max values for M and the frequency range for the VCO_out
calculator were incorrect.  This information was contradicted in other
parts of the mini, nano and plus manuals.  After reaching out to my
NXP Rep, when confronting him about discrepencies in the Nano manual,
he responded with:
 "Yes it is definitely wrong, the one that is part
  of the NOTE in MIPI_DPHY_M_PLLPMS register table against PMS_P,
  PMS_M and PMS_S is not correct. I will report this to Doc team,
  the one customer should be take into account is the Table 13-40
  DPHY PLL Parameters and the Note above."

These updated values also match what is used in the NXP downstream
kernel.

To fix this, make new variables to hold the min and max values of m
and the minimum value of VCO_out, and update the PMS calculator to
use these new variables instead of using hard-coded values to keep
the backwards compatibility with other parts using this driver.

Fixes: 4d562c70c4dc ("drm: bridge: samsung-dsim: Add i.MX8M Mini/Nano support")
Signed-off-by: Adam Ford <aford173@gmail.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 22 ++++++++++++++++++++--
 include/drm/bridge/samsung-dsim.h     |  3 +++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 2be3b58624c3..bf4b33d2de76 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -405,6 +405,9 @@ static const struct samsung_dsim_driver_data exynos3_dsi_driver_data = {
 	.num_bits_resol = 11,
 	.pll_p_offset = 13,
 	.reg_values = reg_values,
+	.m_min = 41,
+	.m_max = 125,
+	.min_freq = 500,
 };
 
 static const struct samsung_dsim_driver_data exynos4_dsi_driver_data = {
@@ -418,6 +421,9 @@ static const struct samsung_dsim_driver_data exynos4_dsi_driver_data = {
 	.num_bits_resol = 11,
 	.pll_p_offset = 13,
 	.reg_values = reg_values,
+	.m_min = 41,
+	.m_max = 125,
+	.min_freq = 500,
 };
 
 static const struct samsung_dsim_driver_data exynos5_dsi_driver_data = {
@@ -429,6 +435,9 @@ static const struct samsung_dsim_driver_data exynos5_dsi_driver_data = {
 	.num_bits_resol = 11,
 	.pll_p_offset = 13,
 	.reg_values = reg_values,
+	.m_min = 41,
+	.m_max = 125,
+	.min_freq = 500,
 };
 
 static const struct samsung_dsim_driver_data exynos5433_dsi_driver_data = {
@@ -441,6 +450,9 @@ static const struct samsung_dsim_driver_data exynos5433_dsi_driver_data = {
 	.num_bits_resol = 12,
 	.pll_p_offset = 13,
 	.reg_values = exynos5433_reg_values,
+	.m_min = 41,
+	.m_max = 125,
+	.min_freq = 500,
 };
 
 static const struct samsung_dsim_driver_data exynos5422_dsi_driver_data = {
@@ -453,6 +465,9 @@ static const struct samsung_dsim_driver_data exynos5422_dsi_driver_data = {
 	.num_bits_resol = 12,
 	.pll_p_offset = 13,
 	.reg_values = exynos5422_reg_values,
+	.m_min = 41,
+	.m_max = 125,
+	.min_freq = 500,
 };
 
 static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
@@ -469,6 +484,9 @@ static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
 	 */
 	.pll_p_offset = 14,
 	.reg_values = imx8mm_dsim_reg_values,
+	.m_min = 64,
+	.m_max = 1023,
+	.min_freq = 1050,
 };
 
 static const struct samsung_dsim_driver_data *
@@ -547,12 +565,12 @@ static unsigned long samsung_dsim_pll_find_pms(struct samsung_dsim *dsi,
 			tmp = (u64)fout * (_p << _s);
 			do_div(tmp, fin);
 			_m = tmp;
-			if (_m < 41 || _m > 125)
+			if (_m < driver_data->m_min || _m > driver_data->m_max)
 				continue;
 
 			tmp = (u64)_m * fin;
 			do_div(tmp, _p);
-			if (tmp < 500 * MHZ ||
+			if (tmp < driver_data->min_freq  * MHZ ||
 			    tmp > driver_data->max_freq * MHZ)
 				continue;
 
diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
index ba5484de2b30..a1a5b2b89a7a 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -54,11 +54,14 @@ struct samsung_dsim_driver_data {
 	unsigned int has_freqband:1;
 	unsigned int has_clklane_stop:1;
 	unsigned int num_clks;
+	unsigned int min_freq;
 	unsigned int max_freq;
 	unsigned int wait_for_reset;
 	unsigned int num_bits_resol;
 	unsigned int pll_p_offset;
 	const unsigned int *reg_values;
+	u16 m_min;
+	u16 m_max;
 };
 
 struct samsung_dsim_host_ops {
-- 
2.39.2

[PATCH V6 3/6] drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically

[Adam Ford]

Make the pll-clock-frequency optional.  If it's present, use it
to maintain backwards compatibility with existing hardware.  If it
is absent, read clock rate of "sclk_mipi" to determine the rate.
Since it can be optional, change the message from an error to
dev_info.

Signed-off-by: Adam Ford <aford173@gmail.com>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index bf4b33d2de76..08266303c261 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1712,11 +1712,11 @@ static const struct mipi_dsi_host_ops samsung_dsim_ops = {
 };
 
 static int samsung_dsim_of_read_u32(const struct device_node *np,
-				    const char *propname, u32 *out_value)
+				    const char *propname, u32 *out_value, bool optional)
 {
 	int ret = of_property_read_u32(np, propname, out_value);
 
-	if (ret < 0)
+	if (ret < 0 && !optional)
 		pr_err("%pOF: failed to get '%s' property\n", np, propname);
 
 	return ret;
@@ -1726,20 +1726,29 @@ static int samsung_dsim_parse_dt(struct samsung_dsim *dsi)
 {
 	struct device *dev = dsi->dev;
 	struct device_node *node = dev->of_node;
+	struct clk *pll_clk;
 	int ret;
 
 	ret = samsung_dsim_of_read_u32(node, "samsung,pll-clock-frequency",
-				       &dsi->pll_clk_rate);
-	if (ret < 0)
-		return ret;
+				       &dsi->pll_clk_rate, 1);
+
+	/* If it doesn't exist, read it from the clock instead of failing */
+	if (ret < 0) {
+		dev_info(dev, "Using sclk_mipi for pll clock frequency\n");
+		pll_clk = devm_clk_get(dev, "sclk_mipi");
+		if (!IS_ERR(pll_clk))
+			dsi->pll_clk_rate = clk_get_rate(pll_clk);
+		else
+			return PTR_ERR(pll_clk);
+	}
 
 	ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-frequency",
-				       &dsi->burst_clk_rate);
+				       &dsi->burst_clk_rate, 0);
 	if (ret < 0)
 		return ret;
 
 	ret = samsung_dsim_of_read_u32(node, "samsung,esc-clock-frequency",
-				       &dsi->esc_clk_rate);
+				       &dsi->esc_clk_rate, 0);
 	if (ret < 0)
 		return ret;
 
-- 
2.39.2

[PATCH V6 4/6] drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY

[Adam Ford]

In order to support variable DPHY timings, it's necessary
to enable GENERIC_PHY_MIPI_DPHY so phy_mipi_dphy_get_default_config
can be used to determine the nominal values for a given resolution
and refresh rate.

Signed-off-by: Adam Ford <aford173@gmail.com>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/gpu/drm/bridge/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index f076a09afac0..82c68b042444 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -227,6 +227,7 @@ config DRM_SAMSUNG_DSIM
 	select DRM_KMS_HELPER
 	select DRM_MIPI_DSI
 	select DRM_PANEL_BRIDGE
+	select GENERIC_PHY_MIPI_DPHY
 	help
 	  The Samsung MIPI DSIM bridge controller driver.
 	  This MIPI DSIM bridge can be found it on Exynos SoCs and
-- 
2.39.2

[PATCH V6 5/6] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Adam Ford]

The DPHY timings are currently hard coded. Since the input
clock can be variable, the phy timings need to be variable
too.  To facilitate this, we need to cache the hs_clock
based on what is generated from the PLL.

The phy_mipi_dphy_get_default_config_for_hsclk function
configures the DPHY timings in pico-seconds, and a small macro
converts those timings into clock cycles based on the hs_clk.

Signed-off-by: Adam Ford <aford173@gmail.com>
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Tested-by: Michael Walle <michael@walle.cc>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 57 +++++++++++++++++++++++----
 include/drm/bridge/samsung-dsim.h     |  1 +
 2 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 08266303c261..3944b7cfbbdf 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -218,6 +218,8 @@
 
 #define OLD_SCLK_MIPI_CLK_NAME		"pll_clk"
 
+#define PS_TO_CYCLE(ps, hz) DIV64_U64_ROUND_CLOSEST(((ps) * (hz)), 1000000000000ULL)
+
 static const char *const clk_names[5] = {
 	"bus_clk",
 	"sclk_mipi",
@@ -651,6 +653,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
 		reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
 	} while ((reg & DSIM_PLL_STABLE) == 0);
 
+	dsi->hs_clock = fout;
+
 	return fout;
 }
 
@@ -698,13 +702,46 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
 	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
 	const unsigned int *reg_values = driver_data->reg_values;
 	u32 reg;
+	struct phy_configure_opts_mipi_dphy cfg;
+	int clk_prepare, lpx, clk_zero, clk_post, clk_trail;
+	int hs_exit, hs_prepare, hs_zero, hs_trail;
+	unsigned long long byte_clock = dsi->hs_clock / 8;
 
 	if (driver_data->has_freqband)
 		return;
 
+	phy_mipi_dphy_get_default_config_for_hsclk(dsi->hs_clock,
+						   dsi->lanes, &cfg);
+
+	/*
+	 * TODO:
+	 * The tech reference manual for i.MX8M Mini/Nano/Plus
+	 * doesn't state what the definition of the PHYTIMING
+	 * bits are beyond their address and bit position.
+	 * After reviewing NXP's downstream code, it appears
+	 * that the various PHYTIMING registers take the number
+	 * of cycles and use various dividers on them.  This
+	 * calculation does not result in an exact match to the
+	 * downstream code, but it is very close, and it appears
+	 * to sync at a variety of resolutions. If someone
+	 * can get a more accurate mathematical equation needed
+	 * for these registers, this should be updated.
+	 */
+
+	lpx = PS_TO_CYCLE(cfg.lpx, byte_clock);
+	hs_exit = PS_TO_CYCLE(cfg.hs_exit, byte_clock);
+	clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, byte_clock);
+	clk_zero = PS_TO_CYCLE(cfg.clk_zero, byte_clock);
+	clk_post = PS_TO_CYCLE(cfg.clk_post, byte_clock);
+	clk_trail = PS_TO_CYCLE(cfg.clk_trail, byte_clock);
+	hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, byte_clock);
+	hs_zero = PS_TO_CYCLE(cfg.hs_zero, byte_clock);
+	hs_trail = PS_TO_CYCLE(cfg.hs_trail, byte_clock);
+
 	/* B D-PHY: D-PHY Master & Slave Analog Block control */
 	reg = reg_values[PHYCTRL_ULPS_EXIT] | reg_values[PHYCTRL_VREG_LP] |
 		reg_values[PHYCTRL_SLEW_UP];
+
 	samsung_dsim_write(dsi, DSIM_PHYCTRL_REG, reg);
 
 	/*
@@ -712,7 +749,9 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
 	 * T HS-EXIT: Time that the transmitter drives LP-11 following a HS
 	 *	burst
 	 */
-	reg = reg_values[PHYTIMING_LPX] | reg_values[PHYTIMING_HS_EXIT];
+
+	reg  = DSIM_PHYTIMING_LPX(lpx) | DSIM_PHYTIMING_HS_EXIT(hs_exit);
+
 	samsung_dsim_write(dsi, DSIM_PHYTIMING_REG, reg);
 
 	/*
@@ -728,10 +767,11 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
 	 * T CLK-TRAIL: Time that the transmitter drives the HS-0 state after
 	 *	the last payload clock bit of a HS transmission burst
 	 */
-	reg = reg_values[PHYTIMING_CLK_PREPARE] |
-		reg_values[PHYTIMING_CLK_ZERO] |
-		reg_values[PHYTIMING_CLK_POST] |
-		reg_values[PHYTIMING_CLK_TRAIL];
+
+	reg = DSIM_PHYTIMING1_CLK_PREPARE(clk_prepare)	|
+	      DSIM_PHYTIMING1_CLK_ZERO(clk_zero)	|
+	      DSIM_PHYTIMING1_CLK_POST(clk_post)	|
+	      DSIM_PHYTIMING1_CLK_TRAIL(clk_trail);
 
 	samsung_dsim_write(dsi, DSIM_PHYTIMING1_REG, reg);
 
@@ -744,8 +784,11 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
 	 * T HS-TRAIL: Time that the transmitter drives the flipped differential
 	 *	state after last payload data bit of a HS transmission burst
 	 */
-	reg = reg_values[PHYTIMING_HS_PREPARE] | reg_values[PHYTIMING_HS_ZERO] |
-		reg_values[PHYTIMING_HS_TRAIL];
+
+	reg = DSIM_PHYTIMING2_HS_PREPARE(hs_prepare) |
+	      DSIM_PHYTIMING2_HS_ZERO(hs_zero) |
+	      DSIM_PHYTIMING2_HS_TRAIL(hs_trail);
+
 	samsung_dsim_write(dsi, DSIM_PHYTIMING2_REG, reg);
 }
 
diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
index a1a5b2b89a7a..d9d431e3b65a 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -93,6 +93,7 @@ struct samsung_dsim {
 
 	u32 pll_clk_rate;
 	u32 burst_clk_rate;
+	u32 hs_clock;
 	u32 esc_clk_rate;
 	u32 lanes;
 	u32 mode_flags;
-- 
2.39.2

[PATCH V6 6/6] drm: bridge: samsung-dsim: Support non-burst mode

[Adam Ford]

The high-speed clock is hard-coded to the burst-clock
frequency specified in the device tree.  However, when
using devices like certain bridge chips without burst mode
and varying resolutions and refresh rates, it may be
necessary to set the high-speed clock dynamically based
on the desired pixel clock for the connected device.

This also removes the need to set a clock speed from
the device tree for non-burst mode operation, since the
pixel clock rate is the rate requested from the attached
device like a bridge chip.  This should have no impact
for people using burst-mode and setting the burst clock
rate is still required for those users.  If the burst
clock is not present, change the error message to
dev_info indicating the clock use the pixel clock.

Signed-off-by: Adam Ford <aford173@gmail.com>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 3944b7cfbbdf..03b21d13f067 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -655,16 +655,28 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
 
 	dsi->hs_clock = fout;
 
+	dsi->hs_clock = fout;
+
 	return fout;
 }
 
 static int samsung_dsim_enable_clock(struct samsung_dsim *dsi)
 {
-	unsigned long hs_clk, byte_clk, esc_clk;
+	unsigned long hs_clk, byte_clk, esc_clk, pix_clk;
 	unsigned long esc_div;
 	u32 reg;
+	struct drm_display_mode *m = &dsi->mode;
+	int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
+
+	/* m->clock is in KHz */
+	pix_clk = m->clock * 1000;
+
+	/* Use burst_clk_rate if available, otherwise use the pix_clk */
+	if (dsi->burst_clk_rate)
+		hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
+	else
+		hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes));
 
-	hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
 	if (!hs_clk) {
 		dev_err(dsi->dev, "failed to configure DSI PLL\n");
 		return -EFAULT;
@@ -935,7 +947,7 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi)
 	u32 reg;
 
 	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
-		int byte_clk_khz = dsi->burst_clk_rate / 1000 / 8;
+		int byte_clk_khz = dsi->hs_clock / 1000 / 8;
 		int hfp = (m->hsync_start - m->hdisplay) * byte_clk_khz / m->clock;
 		int hbp = (m->htotal - m->hsync_end) * byte_clk_khz / m->clock;
 		int hsa = (m->hsync_end - m->hsync_start) * byte_clk_khz / m->clock;
@@ -1785,10 +1797,13 @@ static int samsung_dsim_parse_dt(struct samsung_dsim *dsi)
 			return PTR_ERR(pll_clk);
 	}
 
+	/* If it doesn't exist, use pixel clock instead of failing */
 	ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-frequency",
-				       &dsi->burst_clk_rate, 0);
-	if (ret < 0)
-		return ret;
+				       &dsi->burst_clk_rate, 1);
+	if (ret < 0) {
+		dev_info(dev, "Using pixel clock for HS clock frequency\n");
+		dsi->burst_clk_rate = 0;
+	}
 
 	ret = samsung_dsim_of_read_u32(node, "samsung,esc-clock-frequency",
 				       &dsi->esc_clk_rate, 0);
-- 
2.39.2

Re: [PATCH V6 6/6] drm: bridge: samsung-dsim: Support non-burst mode

[Chen-Yu Tsai]

On Tue, May 16, 2023 at 7:57 AM Adam Ford <aford173@gmail.com> wrote:
>
> The high-speed clock is hard-coded to the burst-clock
> frequency specified in the device tree.  However, when
> using devices like certain bridge chips without burst mode
> and varying resolutions and refresh rates, it may be
> necessary to set the high-speed clock dynamically based
> on the desired pixel clock for the connected device.
>
> This also removes the need to set a clock speed from
> the device tree for non-burst mode operation, since the
> pixel clock rate is the rate requested from the attached
> device like a bridge chip.  This should have no impact
> for people using burst-mode and setting the burst clock
> rate is still required for those users.  If the burst
> clock is not present, change the error message to
> dev_info indicating the clock use the pixel clock.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 3944b7cfbbdf..03b21d13f067 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -655,16 +655,28 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>
>         dsi->hs_clock = fout;
>
> +       dsi->hs_clock = fout;
> +

Not sure about the double assignment. Was this caused by a rebase?

ChenYu

>         return fout;
>  }
>
>  static int samsung_dsim_enable_clock(struct samsung_dsim *dsi)
>  {
> -       unsigned long hs_clk, byte_clk, esc_clk;
> +       unsigned long hs_clk, byte_clk, esc_clk, pix_clk;
>         unsigned long esc_div;
>         u32 reg;
> +       struct drm_display_mode *m = &dsi->mode;
> +       int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> +
> +       /* m->clock is in KHz */
> +       pix_clk = m->clock * 1000;
> +
> +       /* Use burst_clk_rate if available, otherwise use the pix_clk */
> +       if (dsi->burst_clk_rate)
> +               hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
> +       else
> +               hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes));
>
> -       hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
>         if (!hs_clk) {
>                 dev_err(dsi->dev, "failed to configure DSI PLL\n");
>                 return -EFAULT;
> @@ -935,7 +947,7 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi)
>         u32 reg;
>
>         if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
> -               int byte_clk_khz = dsi->burst_clk_rate / 1000 / 8;
> +               int byte_clk_khz = dsi->hs_clock / 1000 / 8;
>                 int hfp = (m->hsync_start - m->hdisplay) * byte_clk_khz / m->clock;
>                 int hbp = (m->htotal - m->hsync_end) * byte_clk_khz / m->clock;
>                 int hsa = (m->hsync_end - m->hsync_start) * byte_clk_khz / m->clock;
> @@ -1785,10 +1797,13 @@ static int samsung_dsim_parse_dt(struct samsung_dsim *dsi)
>                         return PTR_ERR(pll_clk);
>         }
>
> +       /* If it doesn't exist, use pixel clock instead of failing */
>         ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-frequency",
> -                                      &dsi->burst_clk_rate, 0);
> -       if (ret < 0)
> -               return ret;
> +                                      &dsi->burst_clk_rate, 1);
> +       if (ret < 0) {
> +               dev_info(dev, "Using pixel clock for HS clock frequency\n");
> +               dsi->burst_clk_rate = 0;
> +       }
>
>         ret = samsung_dsim_of_read_u32(node, "samsung,esc-clock-frequency",
>                                        &dsi->esc_clk_rate, 0);
> --
> 2.39.2
>

Re: [PATCH V6 6/6] drm: bridge: samsung-dsim: Support non-burst mode

[Adam Ford]

On Mon, May 15, 2023 at 10:26 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Tue, May 16, 2023 at 7:57 AM Adam Ford <aford173@gmail.com> wrote:
> >
> > The high-speed clock is hard-coded to the burst-clock
> > frequency specified in the device tree.  However, when
> > using devices like certain bridge chips without burst mode
> > and varying resolutions and refresh rates, it may be
> > necessary to set the high-speed clock dynamically based
> > on the desired pixel clock for the connected device.
> >
> > This also removes the need to set a clock speed from
> > the device tree for non-burst mode operation, since the
> > pixel clock rate is the rate requested from the attached
> > device like a bridge chip.  This should have no impact
> > for people using burst-mode and setting the burst clock
> > rate is still required for those users.  If the burst
> > clock is not present, change the error message to
> > dev_info indicating the clock use the pixel clock.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> > Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 27 +++++++++++++++++++++------
> >  1 file changed, 21 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index 3944b7cfbbdf..03b21d13f067 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -655,16 +655,28 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
> >
> >         dsi->hs_clock = fout;
> >
> > +       dsi->hs_clock = fout;
> > +
>
> Not sure about the double assignment. Was this caused by a rebase?

Oops,

I moved this to the previous patch since the updated dphy changes
needed to know the hs_clock. I must forgot to check this when I
applied the subsequent patch, so the double assignment appeared.  I am
surprised the patch tool didn't complain.  I guess the good news is
that nothing is broken, but the bad news is I have to spam everyone
with a V7.  I'll wait a couple days to see if anything finds anything
else.

adam
>
> ChenYu
>
> >         return fout;
> >  }
> >
> >  static int samsung_dsim_enable_clock(struct samsung_dsim *dsi)
> >  {
> > -       unsigned long hs_clk, byte_clk, esc_clk;
> > +       unsigned long hs_clk, byte_clk, esc_clk, pix_clk;
> >         unsigned long esc_div;
> >         u32 reg;
> > +       struct drm_display_mode *m = &dsi->mode;
> > +       int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> > +
> > +       /* m->clock is in KHz */
> > +       pix_clk = m->clock * 1000;
> > +
> > +       /* Use burst_clk_rate if available, otherwise use the pix_clk */
> > +       if (dsi->burst_clk_rate)
> > +               hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
> > +       else
> > +               hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes));
> >
> > -       hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
> >         if (!hs_clk) {
> >                 dev_err(dsi->dev, "failed to configure DSI PLL\n");
> >                 return -EFAULT;
> > @@ -935,7 +947,7 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi)
> >         u32 reg;
> >
> >         if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
> > -               int byte_clk_khz = dsi->burst_clk_rate / 1000 / 8;
> > +               int byte_clk_khz = dsi->hs_clock / 1000 / 8;
> >                 int hfp = (m->hsync_start - m->hdisplay) * byte_clk_khz / m->clock;
> >                 int hbp = (m->htotal - m->hsync_end) * byte_clk_khz / m->clock;
> >                 int hsa = (m->hsync_end - m->hsync_start) * byte_clk_khz / m->clock;
> > @@ -1785,10 +1797,13 @@ static int samsung_dsim_parse_dt(struct samsung_dsim *dsi)
> >                         return PTR_ERR(pll_clk);
> >         }
> >
> > +       /* If it doesn't exist, use pixel clock instead of failing */
> >         ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-frequency",
> > -                                      &dsi->burst_clk_rate, 0);
> > -       if (ret < 0)
> > -               return ret;
> > +                                      &dsi->burst_clk_rate, 1);
> > +       if (ret < 0) {
> > +               dev_info(dev, "Using pixel clock for HS clock frequency\n");
> > +               dsi->burst_clk_rate = 0;
> > +       }
> >
> >         ret = samsung_dsim_of_read_u32(node, "samsung,esc-clock-frequency",
> >                                        &dsi->esc_clk_rate, 0);
> > --
> > 2.39.2
> >

Re: [PATCH V6 0/6] drm: bridge: samsung-dsim: Support variable clocking

[Marek Szyprowski]

On 16.05.2023 01:57, Adam Ford wrote:
> This series fixes the blanking pack size and the PMS calculation.  It then
> adds support to allows the DSIM to dynamically DPHY clocks, and support
> non-burst mode while allowing the removal of the hard-coded clock values
> for the PLL for imx8m mini/nano/plus, and it allows the removal of the
> burst-clock device tree entry when burst-mode isn't supported by connected
> devices like an HDMI brige.  In that event, the HS clock is set to the
> value requested by the bridge chip.
>
> This has been tested on both an i.MX8M Nano and i.MX8M Plus, and should
> work on i.MX8M Mini as well. Marek Szyprowski has tested it on various
> Exynos boards.
>
> Adam Ford (5):
>    drm: bridge: samsung-dsim: Fix PMS Calculator on imx8m[mnp]
>    drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically
>    drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY
>    drm: bridge: samsung-dsim: Dynamically configure DPHY timing
>    drm: bridge: samsung-dsim: Support non-burst mode
>
> Lucas Stach (1):
>    drm: bridge: samsung-dsim: fix blanking packet size calculation
>
>   drivers/gpu/drm/bridge/Kconfig        |   1 +
>   drivers/gpu/drm/bridge/samsung-dsim.c | 143 +++++++++++++++++++++-----
>   include/drm/bridge/samsung-dsim.h     |   4 +
>   3 files changed, 125 insertions(+), 23 deletions(-)

Feel free to add to all patches:

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>


> V6:  Squash-in an additional error fix from Lucas Stach regarding the
>       DPHY calcuations.  Remove the dynamic_dphy variable and let
>       everyone use the new calculations.  Move the hs_clock caching
>       from patch 6 to patch 5 to go along with the DPHY calcuations
>       since they are now based on the recorded hs_clock rate.
>       
> V5:  Update error message to dev_info and change them to indicate
>       what is happening without sounding like an error when optional
>       device tree entries are missing.
>
> V4:  Undo some accidental whitespace changes, rename PS_TO_CYCLE
>       variables to ps and hz from PS and MHz. Remove if check
>       before the samsung_dsim_set_phy_ctrl call since it's
>       unnecessary.
>       Added additional tested-by and reviewed-by comments.
>       Squash patches 6 and 7 together since the supporting
>       non-burst (patch 6) mode doesn't really work until
>       patch 7 was applied.
>
> V3:  When checking if the bust-clock is present, only check for it
>       in the device tree, and don't check the presence of the
>       MIPI_DSI_MODE_VIDEO_BURST flag as it breaks an existing Exynos
>       board.
>
>       Add a new patch to the series to select GENERIC_PHY_MIPI_DPHY in
>       Kconfig otherwise the build breaks on the 32-bit Exynos.
>
>       Change vco_min variable name to min_freq
>
>       Added tested-by from Chen-Yu Tsai
>
> V2:  Instead of using my packet blanking calculation, this integrates
>       on from Lucas Stach which gets modified later in the series to
>       cache the value of the HS-clock instead of having to do the
>       calucations again.
>
>       Instead of completely eliminating the PLL clock frequency from
>       the device tree, this makes it optional to avoid breaking some
>       Samsung devices.  When the samsung,pll-clock-frequency is not
>       found, it reads the value of the clock named "sclk_mipi"
>       This also maintains backwards compatibility with older device
>       trees.
>
>       This also changes the DPHY calcuation from a Look-up table,
>       a reverse engineered algorithm which uses
>       phy_mipi_dphy_get_default_config to determine the standard
>       nominal values and calculates the cycles necessary to update
>       the DPHY timings accordingly.
>       

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH V6 5/6] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Adam Ford]

On Mon, May 15, 2023 at 6:57 PM Adam Ford <aford173@gmail.com> wrote:
>
> The DPHY timings are currently hard coded. Since the input
> clock can be variable, the phy timings need to be variable
> too.  To facilitate this, we need to cache the hs_clock
> based on what is generated from the PLL.
>
> The phy_mipi_dphy_get_default_config_for_hsclk function
> configures the DPHY timings in pico-seconds, and a small macro
> converts those timings into clock cycles based on the hs_clk.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Tested-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 57 +++++++++++++++++++++++----
>  include/drm/bridge/samsung-dsim.h     |  1 +
>  2 files changed, 51 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 08266303c261..3944b7cfbbdf 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -218,6 +218,8 @@
>
>  #define OLD_SCLK_MIPI_CLK_NAME         "pll_clk"
>
> +#define PS_TO_CYCLE(ps, hz) DIV64_U64_ROUND_CLOSEST(((ps) * (hz)), 1000000000000ULL)
> +
>  static const char *const clk_names[5] = {
>         "bus_clk",
>         "sclk_mipi",
> @@ -651,6 +653,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>                 reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
>         } while ((reg & DSIM_PLL_STABLE) == 0);
>
> +       dsi->hs_clock = fout;
> +
>         return fout;
>  }
>
> @@ -698,13 +702,46 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>         const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
>         const unsigned int *reg_values = driver_data->reg_values;
>         u32 reg;
> +       struct phy_configure_opts_mipi_dphy cfg;
> +       int clk_prepare, lpx, clk_zero, clk_post, clk_trail;
> +       int hs_exit, hs_prepare, hs_zero, hs_trail;
> +       unsigned long long byte_clock = dsi->hs_clock / 8;
>
>         if (driver_data->has_freqband)
>                 return;
>
> +       phy_mipi_dphy_get_default_config_for_hsclk(dsi->hs_clock,
> +                                                  dsi->lanes, &cfg);
> +
> +       /*
> +        * TODO:
> +        * The tech reference manual for i.MX8M Mini/Nano/Plus
> +        * doesn't state what the definition of the PHYTIMING
> +        * bits are beyond their address and bit position.
> +        * After reviewing NXP's downstream code, it appears
> +        * that the various PHYTIMING registers take the number
> +        * of cycles and use various dividers on them.  This
> +        * calculation does not result in an exact match to the
> +        * downstream code, but it is very close, and it appears
> +        * to sync at a variety of resolutions. If someone
> +        * can get a more accurate mathematical equation needed
> +        * for these registers, this should be updated.
> +        */

Marek Szyprowski -

I was curious to know if you have any opinion on this TODO note and/or
if you have any stuff you can share about how the values of the
following variables are configured?
> +
> +       lpx = PS_TO_CYCLE(cfg.lpx, byte_clock);
> +       hs_exit = PS_TO_CYCLE(cfg.hs_exit, byte_clock);
> +       clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, byte_clock);
> +       clk_zero = PS_TO_CYCLE(cfg.clk_zero, byte_clock);
> +       clk_post = PS_TO_CYCLE(cfg.clk_post, byte_clock);
> +       clk_trail = PS_TO_CYCLE(cfg.clk_trail, byte_clock);
> +       hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, byte_clock);
> +       hs_zero = PS_TO_CYCLE(cfg.hs_zero, byte_clock);
> +       hs_trail = PS_TO_CYCLE(cfg.hs_trail, byte_clock);
> +

These 'work' but they don't exactly match the NXP reference code, but
they're not significantly different.  The NXP reference manual doesn't
describe how these registers are set, they only publish the register
and bits used.  Since you work for Samsung, I was hoping you might
have inside information to know if this is a reasonable approach.

thanks

adam

>         /* B D-PHY: D-PHY Master & Slave Analog Block control */
>         reg = reg_values[PHYCTRL_ULPS_EXIT] | reg_values[PHYCTRL_VREG_LP] |
>                 reg_values[PHYCTRL_SLEW_UP];
> +
>         samsung_dsim_write(dsi, DSIM_PHYCTRL_REG, reg);
>
>         /*
> @@ -712,7 +749,9 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>          * T HS-EXIT: Time that the transmitter drives LP-11 following a HS
>          *      burst
>          */
> -       reg = reg_values[PHYTIMING_LPX] | reg_values[PHYTIMING_HS_EXIT];
> +
> +       reg  = DSIM_PHYTIMING_LPX(lpx) | DSIM_PHYTIMING_HS_EXIT(hs_exit);
> +
>         samsung_dsim_write(dsi, DSIM_PHYTIMING_REG, reg);
>
>         /*
> @@ -728,10 +767,11 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>          * T CLK-TRAIL: Time that the transmitter drives the HS-0 state after
>          *      the last payload clock bit of a HS transmission burst
>          */
> -       reg = reg_values[PHYTIMING_CLK_PREPARE] |
> -               reg_values[PHYTIMING_CLK_ZERO] |
> -               reg_values[PHYTIMING_CLK_POST] |
> -               reg_values[PHYTIMING_CLK_TRAIL];
> +
> +       reg = DSIM_PHYTIMING1_CLK_PREPARE(clk_prepare)  |
> +             DSIM_PHYTIMING1_CLK_ZERO(clk_zero)        |
> +             DSIM_PHYTIMING1_CLK_POST(clk_post)        |
> +             DSIM_PHYTIMING1_CLK_TRAIL(clk_trail);
>
>         samsung_dsim_write(dsi, DSIM_PHYTIMING1_REG, reg);
>
> @@ -744,8 +784,11 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>          * T HS-TRAIL: Time that the transmitter drives the flipped differential
>          *      state after last payload data bit of a HS transmission burst
>          */
> -       reg = reg_values[PHYTIMING_HS_PREPARE] | reg_values[PHYTIMING_HS_ZERO] |
> -               reg_values[PHYTIMING_HS_TRAIL];
> +
> +       reg = DSIM_PHYTIMING2_HS_PREPARE(hs_prepare) |
> +             DSIM_PHYTIMING2_HS_ZERO(hs_zero) |
> +             DSIM_PHYTIMING2_HS_TRAIL(hs_trail);
> +
>         samsung_dsim_write(dsi, DSIM_PHYTIMING2_REG, reg);
>  }
>
> diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
> index a1a5b2b89a7a..d9d431e3b65a 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -93,6 +93,7 @@ struct samsung_dsim {
>
>         u32 pll_clk_rate;
>         u32 burst_clk_rate;
> +       u32 hs_clock;
>         u32 esc_clk_rate;
>         u32 lanes;
>         u32 mode_flags;
> --
> 2.39.2
>

Re: [PATCH V6 0/6] drm: bridge: samsung-dsim: Support variable clocking

[Adam Ford]

On Tue, May 16, 2023 at 5:57 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 16.05.2023 01:57, Adam Ford wrote:
> > This series fixes the blanking pack size and the PMS calculation.  It then
> > adds support to allows the DSIM to dynamically DPHY clocks, and support
> > non-burst mode while allowing the removal of the hard-coded clock values
> > for the PLL for imx8m mini/nano/plus, and it allows the removal of the
> > burst-clock device tree entry when burst-mode isn't supported by connected
> > devices like an HDMI brige.  In that event, the HS clock is set to the
> > value requested by the bridge chip.
> >
> > This has been tested on both an i.MX8M Nano and i.MX8M Plus, and should
> > work on i.MX8M Mini as well. Marek Szyprowski has tested it on various
> > Exynos boards.
> >
> > Adam Ford (5):
> >    drm: bridge: samsung-dsim: Fix PMS Calculator on imx8m[mnp]
> >    drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically
> >    drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY
> >    drm: bridge: samsung-dsim: Dynamically configure DPHY timing
> >    drm: bridge: samsung-dsim: Support non-burst mode
> >
> > Lucas Stach (1):
> >    drm: bridge: samsung-dsim: fix blanking packet size calculation
> >
> >   drivers/gpu/drm/bridge/Kconfig        |   1 +
> >   drivers/gpu/drm/bridge/samsung-dsim.c | 143 +++++++++++++++++++++-----
> >   include/drm/bridge/samsung-dsim.h     |   4 +
> >   3 files changed, 125 insertions(+), 23 deletions(-)
>
> Feel free to add to all patches:
>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Thanks for all your help testing.  I hope the V7 will be the last
attempt.  I've fixed the repeated declaration in patch 6, and added
your t-b statements to each of the patches with code changes.

I'm hoping to push V7 in a day or two pending any more feedback.

adam
>
>
> > V6:  Squash-in an additional error fix from Lucas Stach regarding the
> >       DPHY calcuations.  Remove the dynamic_dphy variable and let
> >       everyone use the new calculations.  Move the hs_clock caching
> >       from patch 6 to patch 5 to go along with the DPHY calcuations
> >       since they are now based on the recorded hs_clock rate.
> >
> > V5:  Update error message to dev_info and change them to indicate
> >       what is happening without sounding like an error when optional
> >       device tree entries are missing.
> >
> > V4:  Undo some accidental whitespace changes, rename PS_TO_CYCLE
> >       variables to ps and hz from PS and MHz. Remove if check
> >       before the samsung_dsim_set_phy_ctrl call since it's
> >       unnecessary.
> >       Added additional tested-by and reviewed-by comments.
> >       Squash patches 6 and 7 together since the supporting
> >       non-burst (patch 6) mode doesn't really work until
> >       patch 7 was applied.
> >
> > V3:  When checking if the bust-clock is present, only check for it
> >       in the device tree, and don't check the presence of the
> >       MIPI_DSI_MODE_VIDEO_BURST flag as it breaks an existing Exynos
> >       board.
> >
> >       Add a new patch to the series to select GENERIC_PHY_MIPI_DPHY in
> >       Kconfig otherwise the build breaks on the 32-bit Exynos.
> >
> >       Change vco_min variable name to min_freq
> >
> >       Added tested-by from Chen-Yu Tsai
> >
> > V2:  Instead of using my packet blanking calculation, this integrates
> >       on from Lucas Stach which gets modified later in the series to
> >       cache the value of the HS-clock instead of having to do the
> >       calucations again.
> >
> >       Instead of completely eliminating the PLL clock frequency from
> >       the device tree, this makes it optional to avoid breaking some
> >       Samsung devices.  When the samsung,pll-clock-frequency is not
> >       found, it reads the value of the clock named "sclk_mipi"
> >       This also maintains backwards compatibility with older device
> >       trees.
> >
> >       This also changes the DPHY calcuation from a Look-up table,
> >       a reverse engineered algorithm which uses
> >       phy_mipi_dphy_get_default_config to determine the standard
> >       nominal values and calculates the cycles necessary to update
> >       the DPHY timings accordingly.
> >
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Re: [PATCH V6 4/6] drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY

[Jagan Teki]

Hi Adam,

On Tue, May 16, 2023 at 5:27 AM Adam Ford <aford173@gmail.com> wrote:
>
> In order to support variable DPHY timings, it's necessary
> to enable GENERIC_PHY_MIPI_DPHY so phy_mipi_dphy_get_default_config
> can be used to determine the nominal values for a given resolution
> and refresh rate.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>  drivers/gpu/drm/bridge/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index f076a09afac0..82c68b042444 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -227,6 +227,7 @@ config DRM_SAMSUNG_DSIM
>         select DRM_KMS_HELPER
>         select DRM_MIPI_DSI
>         select DRM_PANEL_BRIDGE
> +       select GENERIC_PHY_MIPI_DPHY

Is it really required? phy is optional as it is not required for
imx8mm/n/p as of now. May be we can add it while supporting it.

Thanks,
Jagan.

Re: [PATCH V6 4/6] drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY

[Adam Ford]

On Wed, May 17, 2023 at 6:05 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> Hi Adam,
>
> On Tue, May 16, 2023 at 5:27 AM Adam Ford <aford173@gmail.com> wrote:
> >
> > In order to support variable DPHY timings, it's necessary
> > to enable GENERIC_PHY_MIPI_DPHY so phy_mipi_dphy_get_default_config
> > can be used to determine the nominal values for a given resolution
> > and refresh rate.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >  drivers/gpu/drm/bridge/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index f076a09afac0..82c68b042444 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -227,6 +227,7 @@ config DRM_SAMSUNG_DSIM
> >         select DRM_KMS_HELPER
> >         select DRM_MIPI_DSI
> >         select DRM_PANEL_BRIDGE
> > +       select GENERIC_PHY_MIPI_DPHY
>
> Is it really required? phy is optional as it is not required for
> imx8mm/n/p as of now. May be we can add it while supporting it.

This was added to the series because build errors were reported
without it due to the fact that I added calls to
phy_mipi_dphy_get_default_config_for_hsclk.
Selecting this config option guarantees
phy_mipi_dphy_get_default_config_for_hsclk will be built and removes
the build error for Exynos and some 32-bit builds.

phy_mipi_dphy_get_default_config_for_hsclk sets the DSI configurations
like lpx, hs_exit, clk_prepare, clk_zero, clk_trail, hs_prepare,
hs_zero and hs_trail and those need to be
dynamic in order to functional at various resolutions.  I did try
leaving the hard-coded values you used, and I wasn't successful in
getting much to sync.

adam

>
> Thanks,
> Jagan.

Re: [PATCH V6 4/6] drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY

[Jagan Teki]

On Wed, May 17, 2023 at 4:34 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> Hi Adam,
>
> On Tue, May 16, 2023 at 5:27 AM Adam Ford <aford173@gmail.com> wrote:
> >
> > In order to support variable DPHY timings, it's necessary
> > to enable GENERIC_PHY_MIPI_DPHY so phy_mipi_dphy_get_default_config
> > can be used to determine the nominal values for a given resolution
> > and refresh rate.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >  drivers/gpu/drm/bridge/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index f076a09afac0..82c68b042444 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -227,6 +227,7 @@ config DRM_SAMSUNG_DSIM
> >         select DRM_KMS_HELPER
> >         select DRM_MIPI_DSI
> >         select DRM_PANEL_BRIDGE
> > +       select GENERIC_PHY_MIPI_DPHY
>
> Is it really required? phy is optional as it is not required for
> imx8mm/n/p as of now. May be we can add it while supporting it.

Haa, look like the next patch is using it. sorry.

Thanks,
Jagan.

Re: [PATCH V6 5/6] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Jagan Teki]

On Tue, May 16, 2023 at 5:27 AM Adam Ford <aford173@gmail.com> wrote:
>
> The DPHY timings are currently hard coded. Since the input
> clock can be variable, the phy timings need to be variable
> too.  To facilitate this, we need to cache the hs_clock
> based on what is generated from the PLL.
>
> The phy_mipi_dphy_get_default_config_for_hsclk function
> configures the DPHY timings in pico-seconds, and a small macro
> converts those timings into clock cycles based on the hs_clk.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Tested-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 57 +++++++++++++++++++++++----
>  include/drm/bridge/samsung-dsim.h     |  1 +
>  2 files changed, 51 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 08266303c261..3944b7cfbbdf 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -218,6 +218,8 @@
>
>  #define OLD_SCLK_MIPI_CLK_NAME         "pll_clk"
>
> +#define PS_TO_CYCLE(ps, hz) DIV64_U64_ROUND_CLOSEST(((ps) * (hz)), 1000000000000ULL)
> +
>  static const char *const clk_names[5] = {
>         "bus_clk",
>         "sclk_mipi",
> @@ -651,6 +653,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>                 reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
>         } while ((reg & DSIM_PLL_STABLE) == 0);
>
> +       dsi->hs_clock = fout;
> +
>         return fout;
>  }
>
> @@ -698,13 +702,46 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>         const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
>         const unsigned int *reg_values = driver_data->reg_values;
>         u32 reg;
> +       struct phy_configure_opts_mipi_dphy cfg;
> +       int clk_prepare, lpx, clk_zero, clk_post, clk_trail;
> +       int hs_exit, hs_prepare, hs_zero, hs_trail;
> +       unsigned long long byte_clock = dsi->hs_clock / 8;
>
>         if (driver_data->has_freqband)
>                 return;
>
> +       phy_mipi_dphy_get_default_config_for_hsclk(dsi->hs_clock,
> +                                                  dsi->lanes, &cfg);
> +
> +       /*
> +        * TODO:
> +        * The tech reference manual for i.MX8M Mini/Nano/Plus

Does it mean, Applications Processor Reference Manual? better add it
clear reference.

> +        * doesn't state what the definition of the PHYTIMING
> +        * bits are beyond their address and bit position.
> +        * After reviewing NXP's downstream code, it appears
> +        * that the various PHYTIMING registers take the number
> +        * of cycles and use various dividers on them.  This
> +        * calculation does not result in an exact match to the
> +        * downstream code, but it is very close, and it appears
> +        * to sync at a variety of resolutions. If someone
> +        * can get a more accurate mathematical equation needed
> +        * for these registers, this should be updated.
> +        */
> +
> +       lpx = PS_TO_CYCLE(cfg.lpx, byte_clock);
> +       hs_exit = PS_TO_CYCLE(cfg.hs_exit, byte_clock);
> +       clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, byte_clock);
> +       clk_zero = PS_TO_CYCLE(cfg.clk_zero, byte_clock);
> +       clk_post = PS_TO_CYCLE(cfg.clk_post, byte_clock);
> +       clk_trail = PS_TO_CYCLE(cfg.clk_trail, byte_clock);
> +       hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, byte_clock);
> +       hs_zero = PS_TO_CYCLE(cfg.hs_zero, byte_clock);
> +       hs_trail = PS_TO_CYCLE(cfg.hs_trail, byte_clock);

I think we can do some kind of negotiation has done similar in bsp by
taking inputs from bit_clk  and PLL_1432X table. Did you try this? we
thought this approach while writing dsim to support dynamic dphy.

Thanks,
Jagan.

Re: [PATCH V6 5/6] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Adam Ford]

On Wed, May 17, 2023 at 6:28 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Tue, May 16, 2023 at 5:27 AM Adam Ford <aford173@gmail.com> wrote:
> >
> > The DPHY timings are currently hard coded. Since the input
> > clock can be variable, the phy timings need to be variable
> > too.  To facilitate this, we need to cache the hs_clock
> > based on what is generated from the PLL.
> >
> > The phy_mipi_dphy_get_default_config_for_hsclk function
> > configures the DPHY timings in pico-seconds, and a small macro
> > converts those timings into clock cycles based on the hs_clk.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> > Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > Tested-by: Michael Walle <michael@walle.cc>
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 57 +++++++++++++++++++++++----
> >  include/drm/bridge/samsung-dsim.h     |  1 +
> >  2 files changed, 51 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index 08266303c261..3944b7cfbbdf 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -218,6 +218,8 @@
> >
> >  #define OLD_SCLK_MIPI_CLK_NAME         "pll_clk"
> >
> > +#define PS_TO_CYCLE(ps, hz) DIV64_U64_ROUND_CLOSEST(((ps) * (hz)), 1000000000000ULL)
> > +
> >  static const char *const clk_names[5] = {
> >         "bus_clk",
> >         "sclk_mipi",
> > @@ -651,6 +653,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
> >                 reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
> >         } while ((reg & DSIM_PLL_STABLE) == 0);
> >
> > +       dsi->hs_clock = fout;
> > +
> >         return fout;
> >  }
> >
> > @@ -698,13 +702,46 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
> >         const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
> >         const unsigned int *reg_values = driver_data->reg_values;
> >         u32 reg;
> > +       struct phy_configure_opts_mipi_dphy cfg;
> > +       int clk_prepare, lpx, clk_zero, clk_post, clk_trail;
> > +       int hs_exit, hs_prepare, hs_zero, hs_trail;
> > +       unsigned long long byte_clock = dsi->hs_clock / 8;
> >
> >         if (driver_data->has_freqband)
> >                 return;
> >
> > +       phy_mipi_dphy_get_default_config_for_hsclk(dsi->hs_clock,
> > +                                                  dsi->lanes, &cfg);
> > +
> > +       /*
> > +        * TODO:
> > +        * The tech reference manual for i.MX8M Mini/Nano/Plus
>
> Does it mean, Applications Processor Reference Manual? better add it
> clear reference.

I can do that.

>
> > +        * doesn't state what the definition of the PHYTIMING
> > +        * bits are beyond their address and bit position.
> > +        * After reviewing NXP's downstream code, it appears
> > +        * that the various PHYTIMING registers take the number
> > +        * of cycles and use various dividers on them.  This
> > +        * calculation does not result in an exact match to the
> > +        * downstream code, but it is very close, and it appears
> > +        * to sync at a variety of resolutions. If someone
> > +        * can get a more accurate mathematical equation needed
> > +        * for these registers, this should be updated.
> > +        */
> > +
> > +       lpx = PS_TO_CYCLE(cfg.lpx, byte_clock);
> > +       hs_exit = PS_TO_CYCLE(cfg.hs_exit, byte_clock);
> > +       clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, byte_clock);
> > +       clk_zero = PS_TO_CYCLE(cfg.clk_zero, byte_clock);
> > +       clk_post = PS_TO_CYCLE(cfg.clk_post, byte_clock);
> > +       clk_trail = PS_TO_CYCLE(cfg.clk_trail, byte_clock);
> > +       hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, byte_clock);
> > +       hs_zero = PS_TO_CYCLE(cfg.hs_zero, byte_clock);
> > +       hs_trail = PS_TO_CYCLE(cfg.hs_trail, byte_clock);
>
> I think we can do some kind of negotiation has done similar in bsp by
> taking inputs from bit_clk  and PLL_1432X table. Did you try this? we
> thought this approach while writing dsim to support dynamic dphy.

I originally attempted to implement the lookup table that was used in
the downstream NXP kernel, but I was told to not use a lookup table
but to calculate them directly instead.  I reached out to my NXP rep
and I was told that they could not divulge the contents of these
registers since the DSI driver was under a license from Samsung and
that information was not available outside of an NDA.

When I did the testing for this, I tested a variety of resolutions and
refresh rates and compared the values output from here to those
generated by the NXP lookup table, and they are very close.  From what
I could tell, the variance didn't appear to manifest itself on the
monitors that I tried.  I tried to explain this in the TODO message,
but maybe it wasn't clear.

Marek S tested this on Exynos and he didn't report any regressions.

adam
>
> Thanks,
> Jagan.

Re: [PATCH V6 3/6] drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically

[Lucas Stach]

Hi Adam,

Am Montag, dem 15.05.2023 um 18:57 -0500 schrieb Adam Ford:
> Make the pll-clock-frequency optional.  If it's present, use it
> to maintain backwards compatibility with existing hardware.  If it
> is absent, read clock rate of "sclk_mipi" to determine the rate.
> Since it can be optional, change the message from an error to
> dev_info.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index bf4b33d2de76..08266303c261 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1712,11 +1712,11 @@ static const struct mipi_dsi_host_ops samsung_dsim_ops = {
>  };
>  
>  static int samsung_dsim_of_read_u32(const struct device_node *np,
> -				    const char *propname, u32 *out_value)
> +				    const char *propname, u32 *out_value, bool optional)
>  {
>  	int ret = of_property_read_u32(np, propname, out_value);
>  
> -	if (ret < 0)
> +	if (ret < 0 && !optional)
>  		pr_err("%pOF: failed to get '%s' property\n", np, propname);
>  
>  	return ret;
> @@ -1726,20 +1726,29 @@ static int samsung_dsim_parse_dt(struct samsung_dsim *dsi)
>  {
>  	struct device *dev = dsi->dev;
>  	struct device_node *node = dev->of_node;
> +	struct clk *pll_clk;
>  	int ret;
>  
>  	ret = samsung_dsim_of_read_u32(node, "samsung,pll-clock-frequency",
> -				       &dsi->pll_clk_rate);
> -	if (ret < 0)
> -		return ret;
> +				       &dsi->pll_clk_rate, 1);
> +
> +	/* If it doesn't exist, read it from the clock instead of failing */
> +	if (ret < 0) {
> +		dev_info(dev, "Using sclk_mipi for pll clock frequency\n");

While this is certainly helpful while debugging the driver, I don't
think it warrants a info print. Remove or downgrade to dev_dbg?

On the other hand the changed driver behavior should be documented in
the devicetree binding by moving "samsung,pll-clock-frequency" into the
optional properties and spelling out which clock rate is used when the
property is absent.

Regards,
Lucas

> +		pll_clk = devm_clk_get(dev, "sclk_mipi");
> +		if (!IS_ERR(pll_clk))
> +			dsi->pll_clk_rate = clk_get_rate(pll_clk);
> +		else
> +			return PTR_ERR(pll_clk);
> +	}
>  
>  	ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-frequency",
> -				       &dsi->burst_clk_rate);
> +				       &dsi->burst_clk_rate, 0);
>  	if (ret < 0)
>  		return ret;
>  
>  	ret = samsung_dsim_of_read_u32(node, "samsung,esc-clock-frequency",
> -				       &dsi->esc_clk_rate);
> +				       &dsi->esc_clk_rate, 0);
>  	if (ret < 0)
>  		return ret;
>  

Re: [PATCH V6 4/6] drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY

[Lucas Stach]

Am Montag, dem 15.05.2023 um 18:57 -0500 schrieb Adam Ford:
> In order to support variable DPHY timings, it's necessary
> to enable GENERIC_PHY_MIPI_DPHY so phy_mipi_dphy_get_default_config
> can be used to determine the nominal values for a given resolution
> and refresh rate.
> 
I would just squash this one into the patch introducing the dependency.

Regards,
Lucas

> Signed-off-by: Adam Ford <aford173@gmail.com>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>  drivers/gpu/drm/bridge/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index f076a09afac0..82c68b042444 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -227,6 +227,7 @@ config DRM_SAMSUNG_DSIM
>  	select DRM_KMS_HELPER
>  	select DRM_MIPI_DSI
>  	select DRM_PANEL_BRIDGE
> +	select GENERIC_PHY_MIPI_DPHY
>  	help
>  	  The Samsung MIPI DSIM bridge controller driver.
>  	  This MIPI DSIM bridge can be found it on Exynos SoCs and

Re: [PATCH V6 6/6] drm: bridge: samsung-dsim: Support non-burst mode

[Lucas Stach]

Am Montag, dem 15.05.2023 um 18:57 -0500 schrieb Adam Ford:
> The high-speed clock is hard-coded to the burst-clock
> frequency specified in the device tree.  However, when
> using devices like certain bridge chips without burst mode
> and varying resolutions and refresh rates, it may be
> necessary to set the high-speed clock dynamically based
> on the desired pixel clock for the connected device.
> 
> This also removes the need to set a clock speed from
> the device tree for non-burst mode operation, since the
> pixel clock rate is the rate requested from the attached
> device like a bridge chip. 
> 

Same as with the earlier patch, this needs to be documented in the DT
binding by moving "samsung,burst-clock-frequency" to be a optional
property.

Regards,
Lucas

>  This should have no impact
> for people using burst-mode and setting the burst clock
> rate is still required for those users.  If the burst
> clock is not present, change the error message to
> dev_info indicating the clock use the pixel clock.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 3944b7cfbbdf..03b21d13f067 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -655,16 +655,28 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>  
>  	dsi->hs_clock = fout;
>  
> +	dsi->hs_clock = fout;
> +
>  	return fout;
>  }
>  
>  static int samsung_dsim_enable_clock(struct samsung_dsim *dsi)
>  {
> -	unsigned long hs_clk, byte_clk, esc_clk;
> +	unsigned long hs_clk, byte_clk, esc_clk, pix_clk;
>  	unsigned long esc_div;
>  	u32 reg;
> +	struct drm_display_mode *m = &dsi->mode;
> +	int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> +
> +	/* m->clock is in KHz */
> +	pix_clk = m->clock * 1000;
> +
> +	/* Use burst_clk_rate if available, otherwise use the pix_clk */
> +	if (dsi->burst_clk_rate)
> +		hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
> +	else
> +		hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes));
>  
> -	hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
>  	if (!hs_clk) {
>  		dev_err(dsi->dev, "failed to configure DSI PLL\n");
>  		return -EFAULT;
> @@ -935,7 +947,7 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi)
>  	u32 reg;
>  
>  	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
> -		int byte_clk_khz = dsi->burst_clk_rate / 1000 / 8;
> +		int byte_clk_khz = dsi->hs_clock / 1000 / 8;
>  		int hfp = (m->hsync_start - m->hdisplay) * byte_clk_khz / m->clock;
>  		int hbp = (m->htotal - m->hsync_end) * byte_clk_khz / m->clock;
>  		int hsa = (m->hsync_end - m->hsync_start) * byte_clk_khz / m->clock;
> @@ -1785,10 +1797,13 @@ static int samsung_dsim_parse_dt(struct samsung_dsim *dsi)
>  			return PTR_ERR(pll_clk);
>  	}
>  
> +	/* If it doesn't exist, use pixel clock instead of failing */
>  	ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-frequency",
> -				       &dsi->burst_clk_rate, 0);
> -	if (ret < 0)
> -		return ret;
> +				       &dsi->burst_clk_rate, 1);
> +	if (ret < 0) {
> +		dev_info(dev, "Using pixel clock for HS clock frequency\n");
> +		dsi->burst_clk_rate = 0;
> +	}
>  
>  	ret = samsung_dsim_of_read_u32(node, "samsung,esc-clock-frequency",
>  				       &dsi->esc_clk_rate, 0);

Re: [PATCH V6 4/6] drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY

[Adam Ford]

On Wed, May 17, 2023 at 7:58 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Montag, dem 15.05.2023 um 18:57 -0500 schrieb Adam Ford:
> > In order to support variable DPHY timings, it's necessary
> > to enable GENERIC_PHY_MIPI_DPHY so phy_mipi_dphy_get_default_config
> > can be used to determine the nominal values for a given resolution
> > and refresh rate.
> >
> I would just squash this one into the patch introducing the dependency.

I thought Kconfig updates were supposed to be on their own.  Is that
not correct?

adam
>
> Regards,
> Lucas
>
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >  drivers/gpu/drm/bridge/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index f076a09afac0..82c68b042444 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -227,6 +227,7 @@ config DRM_SAMSUNG_DSIM
> >       select DRM_KMS_HELPER
> >       select DRM_MIPI_DSI
> >       select DRM_PANEL_BRIDGE
> > +     select GENERIC_PHY_MIPI_DPHY
> >       help
> >         The Samsung MIPI DSIM bridge controller driver.
> >         This MIPI DSIM bridge can be found it on Exynos SoCs and
>

Re: [PATCH V6 5/6] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Lucas Stach]

Am Montag, dem 15.05.2023 um 18:57 -0500 schrieb Adam Ford:
> The DPHY timings are currently hard coded. Since the input
> clock can be variable, the phy timings need to be variable
> too.  To facilitate this, we need to cache the hs_clock
> based on what is generated from the PLL.
> 
> The phy_mipi_dphy_get_default_config_for_hsclk function
> configures the DPHY timings in pico-seconds, and a small macro
> converts those timings into clock cycles based on the hs_clk.
> 
I'm not going apply a review tag to a patch where I contributed myself,
but FWIW this looks good to me.

> Signed-off-by: Adam Ford <aford173@gmail.com>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Tested-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 57 +++++++++++++++++++++++----
>  include/drm/bridge/samsung-dsim.h     |  1 +
>  2 files changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 08266303c261..3944b7cfbbdf 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -218,6 +218,8 @@
>  
>  #define OLD_SCLK_MIPI_CLK_NAME		"pll_clk"
>  
> +#define PS_TO_CYCLE(ps, hz) DIV64_U64_ROUND_CLOSEST(((ps) * (hz)), 1000000000000ULL)
> +
>  static const char *const clk_names[5] = {
>  	"bus_clk",
>  	"sclk_mipi",
> @@ -651,6 +653,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>  		reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
>  	} while ((reg & DSIM_PLL_STABLE) == 0);
>  
> +	dsi->hs_clock = fout;
> +
>  	return fout;
>  }
>  
> @@ -698,13 +702,46 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>  	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
>  	const unsigned int *reg_values = driver_data->reg_values;
>  	u32 reg;
> +	struct phy_configure_opts_mipi_dphy cfg;
> +	int clk_prepare, lpx, clk_zero, clk_post, clk_trail;
> +	int hs_exit, hs_prepare, hs_zero, hs_trail;
> +	unsigned long long byte_clock = dsi->hs_clock / 8;
>  
>  	if (driver_data->has_freqband)
>  		return;
>  
> +	phy_mipi_dphy_get_default_config_for_hsclk(dsi->hs_clock,
> +						   dsi->lanes, &cfg);
> +
> +	/*
> +	 * TODO:
> +	 * The tech reference manual for i.MX8M Mini/Nano/Plus
> +	 * doesn't state what the definition of the PHYTIMING
> +	 * bits are beyond their address and bit position.
> +	 * After reviewing NXP's downstream code, it appears
> +	 * that the various PHYTIMING registers take the number
> +	 * of cycles and use various dividers on them.  This
> +	 * calculation does not result in an exact match to the
> +	 * downstream code, but it is very close, and it appears
> +	 * to sync at a variety of resolutions. If someone
> +	 * can get a more accurate mathematical equation needed
> +	 * for these registers, this should be updated.
> +	 */
> +
> +	lpx = PS_TO_CYCLE(cfg.lpx, byte_clock);
> +	hs_exit = PS_TO_CYCLE(cfg.hs_exit, byte_clock);
> +	clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, byte_clock);
> +	clk_zero = PS_TO_CYCLE(cfg.clk_zero, byte_clock);
> +	clk_post = PS_TO_CYCLE(cfg.clk_post, byte_clock);
> +	clk_trail = PS_TO_CYCLE(cfg.clk_trail, byte_clock);
> +	hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, byte_clock);
> +	hs_zero = PS_TO_CYCLE(cfg.hs_zero, byte_clock);
> +	hs_trail = PS_TO_CYCLE(cfg.hs_trail, byte_clock);
> +
>  	/* B D-PHY: D-PHY Master & Slave Analog Block control */
>  	reg = reg_values[PHYCTRL_ULPS_EXIT] | reg_values[PHYCTRL_VREG_LP] |
>  		reg_values[PHYCTRL_SLEW_UP];
> +
>  	samsung_dsim_write(dsi, DSIM_PHYCTRL_REG, reg);
>  
>  	/*
> @@ -712,7 +749,9 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>  	 * T HS-EXIT: Time that the transmitter drives LP-11 following a HS
>  	 *	burst
>  	 */
> -	reg = reg_values[PHYTIMING_LPX] | reg_values[PHYTIMING_HS_EXIT];
> +
> +	reg  = DSIM_PHYTIMING_LPX(lpx) | DSIM_PHYTIMING_HS_EXIT(hs_exit);
> +
>  	samsung_dsim_write(dsi, DSIM_PHYTIMING_REG, reg);
>  
>  	/*
> @@ -728,10 +767,11 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>  	 * T CLK-TRAIL: Time that the transmitter drives the HS-0 state after
>  	 *	the last payload clock bit of a HS transmission burst
>  	 */
> -	reg = reg_values[PHYTIMING_CLK_PREPARE] |
> -		reg_values[PHYTIMING_CLK_ZERO] |
> -		reg_values[PHYTIMING_CLK_POST] |
> -		reg_values[PHYTIMING_CLK_TRAIL];
> +
> +	reg = DSIM_PHYTIMING1_CLK_PREPARE(clk_prepare)	|
> +	      DSIM_PHYTIMING1_CLK_ZERO(clk_zero)	|
> +	      DSIM_PHYTIMING1_CLK_POST(clk_post)	|
> +	      DSIM_PHYTIMING1_CLK_TRAIL(clk_trail);
>  
>  	samsung_dsim_write(dsi, DSIM_PHYTIMING1_REG, reg);
>  
> @@ -744,8 +784,11 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>  	 * T HS-TRAIL: Time that the transmitter drives the flipped differential
>  	 *	state after last payload data bit of a HS transmission burst
>  	 */
> -	reg = reg_values[PHYTIMING_HS_PREPARE] | reg_values[PHYTIMING_HS_ZERO] |
> -		reg_values[PHYTIMING_HS_TRAIL];
> +
> +	reg = DSIM_PHYTIMING2_HS_PREPARE(hs_prepare) |
> +	      DSIM_PHYTIMING2_HS_ZERO(hs_zero) |
> +	      DSIM_PHYTIMING2_HS_TRAIL(hs_trail);
> +
>  	samsung_dsim_write(dsi, DSIM_PHYTIMING2_REG, reg);
>  }
>  
> diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
> index a1a5b2b89a7a..d9d431e3b65a 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -93,6 +93,7 @@ struct samsung_dsim {
>  
>  	u32 pll_clk_rate;
>  	u32 burst_clk_rate;
> +	u32 hs_clock;
>  	u32 esc_clk_rate;
>  	u32 lanes;
>  	u32 mode_flags;

Re: [PATCH V6 3/6] drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically

[Adam Ford]

On Wed, May 17, 2023 at 7:56 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi Adam,
>
> Am Montag, dem 15.05.2023 um 18:57 -0500 schrieb Adam Ford:
> > Make the pll-clock-frequency optional.  If it's present, use it
> > to maintain backwards compatibility with existing hardware.  If it
> > is absent, read clock rate of "sclk_mipi" to determine the rate.
> > Since it can be optional, change the message from an error to
> > dev_info.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> > Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index bf4b33d2de76..08266303c261 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -1712,11 +1712,11 @@ static const struct mipi_dsi_host_ops samsung_dsim_ops = {
> >  };
> >
> >  static int samsung_dsim_of_read_u32(const struct device_node *np,
> > -                                 const char *propname, u32 *out_value)
> > +                                 const char *propname, u32 *out_value, bool optional)
> >  {
> >       int ret = of_property_read_u32(np, propname, out_value);
> >
> > -     if (ret < 0)
> > +     if (ret < 0 && !optional)
> >               pr_err("%pOF: failed to get '%s' property\n", np, propname);
> >
> >       return ret;
> > @@ -1726,20 +1726,29 @@ static int samsung_dsim_parse_dt(struct samsung_dsim *dsi)
> >  {
> >       struct device *dev = dsi->dev;
> >       struct device_node *node = dev->of_node;
> > +     struct clk *pll_clk;
> >       int ret;
> >
> >       ret = samsung_dsim_of_read_u32(node, "samsung,pll-clock-frequency",
> > -                                    &dsi->pll_clk_rate);
> > -     if (ret < 0)
> > -             return ret;
> > +                                    &dsi->pll_clk_rate, 1);
> > +
> > +     /* If it doesn't exist, read it from the clock instead of failing */
> > +     if (ret < 0) {
> > +             dev_info(dev, "Using sclk_mipi for pll clock frequency\n");
>
> While this is certainly helpful while debugging the driver, I don't
> think it warrants a info print. Remove or downgrade to dev_dbg?

I can move to dbg.

>
> On the other hand the changed driver behavior should be documented in
> the devicetree binding by moving "samsung,pll-clock-frequency" into the
> optional properties and spelling out which clock rate is used when the
> property is absent.

Once this series is accepted, I was planning on doing a binding patch
which describes the items that are now optional followed by a patch to
add DSI->HDMI for the Beacon boards.  I can see the value in putting
the bindings patch in this series instead. I'll add it to the next
revision to cover both items that are now optional.

adam

>
> Regards,
> Lucas
>
> > +             pll_clk = devm_clk_get(dev, "sclk_mipi");
> > +             if (!IS_ERR(pll_clk))
> > +                     dsi->pll_clk_rate = clk_get_rate(pll_clk);
> > +             else
> > +                     return PTR_ERR(pll_clk);
> > +     }
> >
> >       ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-frequency",
> > -                                    &dsi->burst_clk_rate);
> > +                                    &dsi->burst_clk_rate, 0);
> >       if (ret < 0)
> >               return ret;
> >
> >       ret = samsung_dsim_of_read_u32(node, "samsung,esc-clock-frequency",
> > -                                    &dsi->esc_clk_rate);
> > +                                    &dsi->esc_clk_rate, 0);
> >       if (ret < 0)
> >               return ret;
> >
>

Re: [PATCH V6 4/6] drm: bridge: samsung-dsim: Select GENERIC_PHY_MIPI_DPHY

[Lucas Stach]

Am Mittwoch, dem 17.05.2023 um 08:02 -0500 schrieb Adam Ford:
> On Wed, May 17, 2023 at 7:58 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > 
> > Am Montag, dem 15.05.2023 um 18:57 -0500 schrieb Adam Ford:
> > > In order to support variable DPHY timings, it's necessary
> > > to enable GENERIC_PHY_MIPI_DPHY so phy_mipi_dphy_get_default_config
> > > can be used to determine the nominal values for a given resolution
> > > and refresh rate.
> > > 
> > I would just squash this one into the patch introducing the dependency.
> 
> I thought Kconfig updates were supposed to be on their own.  Is that
> not correct?
> 
I'm not aware of a general rule for this, but maybe I just missed it.
Personally I would have added this to the patch introducing the
dependency, but I'm also fine with keeping it as a separate patch.

Regards,
Lucas


> adam
> > 
> > Regards,
> > Lucas
> > 
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> > > ---
> > >  drivers/gpu/drm/bridge/Kconfig | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > > index f076a09afac0..82c68b042444 100644
> > > --- a/drivers/gpu/drm/bridge/Kconfig
> > > +++ b/drivers/gpu/drm/bridge/Kconfig
> > > @@ -227,6 +227,7 @@ config DRM_SAMSUNG_DSIM
> > >       select DRM_KMS_HELPER
> > >       select DRM_MIPI_DSI
> > >       select DRM_PANEL_BRIDGE
> > > +     select GENERIC_PHY_MIPI_DPHY
> > >       help
> > >         The Samsung MIPI DSIM bridge controller driver.
> > >         This MIPI DSIM bridge can be found it on Exynos SoCs and
> > 

Re: [PATCH V6 5/6] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Marek Szyprowski]

Hi Adam,

On 17.05.2023 04:55, Adam Ford wrote:
> On Mon, May 15, 2023 at 6:57 PM Adam Ford <aford173@gmail.com> wrote:
>> The DPHY timings are currently hard coded. Since the input
>> clock can be variable, the phy timings need to be variable
>> too.  To facilitate this, we need to cache the hs_clock
>> based on what is generated from the PLL.
>>
>> The phy_mipi_dphy_get_default_config_for_hsclk function
>> configures the DPHY timings in pico-seconds, and a small macro
>> converts those timings into clock cycles based on the hs_clk.
>>
>> Signed-off-by: Adam Ford <aford173@gmail.com>
>> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
>> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> Tested-by: Michael Walle <michael@walle.cc>
>> ---
>>   drivers/gpu/drm/bridge/samsung-dsim.c | 57 +++++++++++++++++++++++----
>>   include/drm/bridge/samsung-dsim.h     |  1 +
>>   2 files changed, 51 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index 08266303c261..3944b7cfbbdf 100644
>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>> @@ -218,6 +218,8 @@
>>
>>   #define OLD_SCLK_MIPI_CLK_NAME         "pll_clk"
>>
>> +#define PS_TO_CYCLE(ps, hz) DIV64_U64_ROUND_CLOSEST(((ps) * (hz)), 1000000000000ULL)
>> +
>>   static const char *const clk_names[5] = {
>>          "bus_clk",
>>          "sclk_mipi",
>> @@ -651,6 +653,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>>                  reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
>>          } while ((reg & DSIM_PLL_STABLE) == 0);
>>
>> +       dsi->hs_clock = fout;
>> +
>>          return fout;
>>   }
>>
>> @@ -698,13 +702,46 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>>          const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
>>          const unsigned int *reg_values = driver_data->reg_values;
>>          u32 reg;
>> +       struct phy_configure_opts_mipi_dphy cfg;
>> +       int clk_prepare, lpx, clk_zero, clk_post, clk_trail;
>> +       int hs_exit, hs_prepare, hs_zero, hs_trail;
>> +       unsigned long long byte_clock = dsi->hs_clock / 8;
>>
>>          if (driver_data->has_freqband)
>>                  return;
>>
>> +       phy_mipi_dphy_get_default_config_for_hsclk(dsi->hs_clock,
>> +                                                  dsi->lanes, &cfg);
>> +
>> +       /*
>> +        * TODO:
>> +        * The tech reference manual for i.MX8M Mini/Nano/Plus
>> +        * doesn't state what the definition of the PHYTIMING
>> +        * bits are beyond their address and bit position.
>> +        * After reviewing NXP's downstream code, it appears
>> +        * that the various PHYTIMING registers take the number
>> +        * of cycles and use various dividers on them.  This
>> +        * calculation does not result in an exact match to the
>> +        * downstream code, but it is very close, and it appears
>> +        * to sync at a variety of resolutions. If someone
>> +        * can get a more accurate mathematical equation needed
>> +        * for these registers, this should be updated.
>> +        */
> Marek Szyprowski -
>
> I was curious to know if you have any opinion on this TODO note and/or
> if you have any stuff you can share about how the values of the
> following variables are configured?
>> +
>> +       lpx = PS_TO_CYCLE(cfg.lpx, byte_clock);
>> +       hs_exit = PS_TO_CYCLE(cfg.hs_exit, byte_clock);
>> +       clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, byte_clock);
>> +       clk_zero = PS_TO_CYCLE(cfg.clk_zero, byte_clock);
>> +       clk_post = PS_TO_CYCLE(cfg.clk_post, byte_clock);
>> +       clk_trail = PS_TO_CYCLE(cfg.clk_trail, byte_clock);
>> +       hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, byte_clock);
>> +       hs_zero = PS_TO_CYCLE(cfg.hs_zero, byte_clock);
>> +       hs_trail = PS_TO_CYCLE(cfg.hs_trail, byte_clock);
>> +
> These 'work' but they don't exactly match the NXP reference code, but
> they're not significantly different.  The NXP reference manual doesn't
> describe how these registers are set, they only publish the register
> and bits used.  Since you work for Samsung, I was hoping you might
> have inside information to know if this is a reasonable approach.

Unfortunately I won't be able to provide any info on that. You may check 
the reference Samsung code for various Exynos based products, but I 
suspect it will be similar to what was already in the Exynos DSI driver.

 > ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH V6 1/6] drm: bridge: samsung-dsim: fix blanking packet size calculation

[Jagan Teki]

On Tue, May 16, 2023 at 5:27 AM Adam Ford <aford173@gmail.com> wrote:
>
> From: Lucas Stach <l.stach@pengutronix.de>
>
> Scale the blanking packet sizes to match the ratio between HS clock
> and DPI interface clock. The controller seems to do internal scaling
> to the number of active lanes, so we don't take those into account.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---

Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
Tested-by: Jagan Teki <jagan@amarulasolutions.com> # imx8mm-icore

Re: [PATCH V6 2/6] drm: bridge: samsung-dsim: Fix PMS Calculator on imx8m[mnp]

[Jagan Teki]

On Tue, May 16, 2023 at 5:27 AM Adam Ford <aford173@gmail.com> wrote:
>
> According to Table 13-45 of the i.MX8M Mini Reference Manual, the min
> and max values for M and the frequency range for the VCO_out
> calculator were incorrect.  This information was contradicted in other
> parts of the mini, nano and plus manuals.  After reaching out to my
> NXP Rep, when confronting him about discrepencies in the Nano manual,
> he responded with:
>  "Yes it is definitely wrong, the one that is part
>   of the NOTE in MIPI_DPHY_M_PLLPMS register table against PMS_P,
>   PMS_M and PMS_S is not correct. I will report this to Doc team,
>   the one customer should be take into account is the Table 13-40
>   DPHY PLL Parameters and the Note above."
>
> These updated values also match what is used in the NXP downstream
> kernel.
>
> To fix this, make new variables to hold the min and max values of m
> and the minimum value of VCO_out, and update the PMS calculator to
> use these new variables instead of using hard-coded values to keep
> the backwards compatibility with other parts using this driver.
>
> Fixes: 4d562c70c4dc ("drm: bridge: samsung-dsim: Add i.MX8M Mini/Nano support")
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---

Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
Tested-by: Jagan Teki <jagan@amarulasolutions.com> # imx8mm-icore

Re: [PATCH V6 3/6] drm: bridge: samsung-dsim: Fetch pll-clock-frequency automatically

[Jagan Teki]

On Tue, May 16, 2023 at 5:27 AM Adam Ford <aford173@gmail.com> wrote:
>
> Make the pll-clock-frequency optional.  If it's present, use it
> to maintain backwards compatibility with existing hardware.  If it
> is absent, read clock rate of "sclk_mipi" to determine the rate.
> Since it can be optional, change the message from an error to
> dev_info.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---

Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
Tested-by: Jagan Teki <jagan@amarulasolutions.com> # imx8mm-icore

Re: [PATCH V6 5/6] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

[Jagan Teki]

On Tue, May 16, 2023 at 5:27 AM Adam Ford <aford173@gmail.com> wrote:
>
> The DPHY timings are currently hard coded. Since the input
> clock can be variable, the phy timings need to be variable
> too.  To facilitate this, we need to cache the hs_clock
> based on what is generated from the PLL.
>
> The phy_mipi_dphy_get_default_config_for_hsclk function
> configures the DPHY timings in pico-seconds, and a small macro
> converts those timings into clock cycles based on the hs_clk.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Tested-by: Michael Walle <michael@walle.cc>
> ---

Tested-by: Jagan Teki <jagan@amarulasolutions.com> # imx8mm-icore

Re: [PATCH V6 6/6] drm: bridge: samsung-dsim: Support non-burst mode

[Jagan Teki]

On Tue, May 16, 2023 at 5:27 AM Adam Ford <aford173@gmail.com> wrote:
>
> The high-speed clock is hard-coded to the burst-clock
> frequency specified in the device tree.  However, when
> using devices like certain bridge chips without burst mode
> and varying resolutions and refresh rates, it may be
> necessary to set the high-speed clock dynamically based
> on the desired pixel clock for the connected device.
>
> This also removes the need to set a clock speed from
> the device tree for non-burst mode operation, since the
> pixel clock rate is the rate requested from the attached
> device like a bridge chip.  This should have no impact
> for people using burst-mode and setting the burst clock
> rate is still required for those users.  If the burst
> clock is not present, change the error message to
> dev_info indicating the clock use the pixel clock.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 3944b7cfbbdf..03b21d13f067 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -655,16 +655,28 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>
>         dsi->hs_clock = fout;
>
> +       dsi->hs_clock = fout;

I dropped this and tested it.

Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
Tested-by: Jagan Teki <jagan@amarulasolutions.com> # imx8mm-icore