[PATCH 0/4] drm/msm/dp: Add support for SC8180x eDP controller

[PATCH 0/4] drm/msm/dp: Add support for SC8180x eDP controller

[Bjorn Andersson]

The first patch in the series is somewhat unrelated to the support, but
simplifies reasoning and debugging of timing related issues.

The second patch introduces support for dealing with different register block
layouts, which is used in the forth patch to describe the hardware blocks found
in the SC8180x eDP block.

The third patch configures the INTF_CONFIG register, which carries the
configuration for widebus handling. As with the DPU the bootloader enables
widebus and we need to disable it, or implement support for adjusting the
timing.

Bjorn Andersson (4):
  drm/msm/dp: Simplify the mvid/nvid calculation
  drm/msm/dp: Store each subblock in the io region
  drm/msm/dp: Initialize the INTF_CONFIG register
  drm/msm/dp: Add support for SC8180x eDP

 drivers/gpu/drm/msm/dp/dp_catalog.c | 99 +++++++----------------------
 drivers/gpu/drm/msm/dp/dp_display.c |  1 +
 drivers/gpu/drm/msm/dp/dp_parser.c  | 22 +++++++
 drivers/gpu/drm/msm/dp/dp_parser.h  |  8 +++
 4 files changed, 53 insertions(+), 77 deletions(-)

-- 
2.29.2

[PATCH 1/4] drm/msm/dp: Simplify the mvid/nvid calculation

[Bjorn Andersson]

In the search for causes to timing issues seen during implementation of
eDP support for SC8180x a fair amount of time was spent concluding why
the calculated mvid/nvid values where wrong.

The overall conclusion is that the ratio of MVID/NVID describes, and
should match, the ratio between the pixel and link clock.

Downstream this calculation reads the M and N values off the pixel clock
straight from DISP_CC and are then adjusted based on knowledge of how
the link and vco_div (parent of the pixel clock) are derrived from the
common VCO.

While upstreaming, and then extracting the PHY driver, the resulting
function performs the following steps:

1) Adjust the passed link rate based on the VCO divider used in the PHY
   driver, and multiply this by 10 based on the link rate divider.
2) Pick reasonable choices of M and N, by calculating the ratio between
   this new clock and the pixel clock.
3) Subtract M from N and flip the bits, to match the encoding of the N
   register in DISP_CC.
4) Flip the bits of N and add M, to get the value of N back.
5) Multiply M with 5, per the documentation.
6) Scale the values such that N is close to 0x8000 (or larger)
7) Multply M with 2 or 3 depending on the link rate of HBR2 or HBR3.

Presumably step 3) was added to provide step 4) with expected input, so
the two cancel each other out. The factor of 10 from step 1) goes into
the denominator and is partially cancelled by the 5 in the numerator in
step 5), resulting in step 7) simply cancelling out step 1).

Left is the code that finds the ratio between the two arguments, scaled
to keep the denominator close to or larger than 0x8000. And this is our
mvid/nvid pair.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_catalog.c | 41 +++++------------------------
 1 file changed, 6 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index b1a9b1b98f5f..2eb37ee48e42 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -415,39 +415,16 @@ void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog,
 					u32 rate, u32 stream_rate_khz,
 					bool fixed_nvid)
 {
-	u32 pixel_m, pixel_n;
-	u32 mvid, nvid, pixel_div = 0, dispcc_input_rate;
 	u32 const nvid_fixed = DP_LINK_CONSTANT_N_VALUE;
-	u32 const link_rate_hbr2 = 540000;
-	u32 const link_rate_hbr3 = 810000;
-	unsigned long den, num;
-
+	unsigned long mvid, nvid;
 	struct dp_catalog_private *catalog = container_of(dp_catalog,
 				struct dp_catalog_private, dp_catalog);
 
-	if (rate == link_rate_hbr3)
-		pixel_div = 6;
-	else if (rate == 1620000 || rate == 270000)
-		pixel_div = 2;
-	else if (rate == link_rate_hbr2)
-		pixel_div = 4;
-	else
-		DRM_ERROR("Invalid pixel mux divider\n");
-
-	dispcc_input_rate = (rate * 10) / pixel_div;
-
-	rational_best_approximation(dispcc_input_rate, stream_rate_khz,
-			(unsigned long)(1 << 16) - 1,
-			(unsigned long)(1 << 16) - 1, &den, &num);
-
-	den = ~(den - num);
-	den = den & 0xFFFF;
-	pixel_m = num;
-	pixel_n = den;
-
-	mvid = (pixel_m & 0xFFFF) * 5;
-	nvid = (0xFFFF & (~pixel_n)) + (pixel_m & 0xFFFF);
+	rational_best_approximation(stream_rate_khz, rate,
+				    (1 << 16) - 1, (1 << 16) - 1,
+				    &mvid, &nvid);
 
+	/* Adjust values so that nvid is close to DP_LINK_CONSTANT_N_VALUE */
 	if (nvid < nvid_fixed) {
 		u32 temp;
 
@@ -456,13 +433,7 @@ void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog,
 		nvid = temp;
 	}
 
-	if (link_rate_hbr2 == rate)
-		nvid *= 2;
-
-	if (link_rate_hbr3 == rate)
-		nvid *= 3;
-
-	DRM_DEBUG_DP("mvid=0x%x, nvid=0x%x\n", mvid, nvid);
+	DRM_DEBUG_DP("mvid=0x%lx, nvid=0x%lx\n", mvid, nvid);
 	dp_write_link(catalog, REG_DP_SOFTWARE_MVID, mvid);
 	dp_write_link(catalog, REG_DP_SOFTWARE_NVID, nvid);
 	dp_write_p0(catalog, MMSS_DP_DSC_DTO, 0x0);
-- 
2.29.2

[PATCH 2/4] drm/msm/dp: Store each subblock in the io region

[Bjorn Andersson]

Not all platforms has DP_P0 at offset 0x1000 from the beginning of the
DP block. So move the offsets into dss_io_data, to make it possible in
the next patch to specify alternative offsets and sizes of these
segments.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_catalog.c | 57 ++++++++---------------------
 drivers/gpu/drm/msm/dp/dp_parser.c  | 10 +++++
 drivers/gpu/drm/msm/dp/dp_parser.h  |  8 ++++
 3 files changed, 33 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 2eb37ee48e42..a0449a2867e4 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -24,15 +24,6 @@
 #define DP_INTERRUPT_STATUS_ACK_SHIFT	1
 #define DP_INTERRUPT_STATUS_MASK_SHIFT	2
 
-#define MSM_DP_CONTROLLER_AHB_OFFSET	0x0000
-#define MSM_DP_CONTROLLER_AHB_SIZE	0x0200
-#define MSM_DP_CONTROLLER_AUX_OFFSET	0x0200
-#define MSM_DP_CONTROLLER_AUX_SIZE	0x0200
-#define MSM_DP_CONTROLLER_LINK_OFFSET	0x0400
-#define MSM_DP_CONTROLLER_LINK_SIZE	0x0C00
-#define MSM_DP_CONTROLLER_P0_OFFSET	0x1000
-#define MSM_DP_CONTROLLER_P0_SIZE	0x0400
-
 #define DP_INTERRUPT_STATUS1 \
 	(DP_INTR_AUX_I2C_DONE| \
 	DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \
@@ -64,75 +55,67 @@ struct dp_catalog_private {
 
 static inline u32 dp_read_aux(struct dp_catalog_private *catalog, u32 offset)
 {
-	offset += MSM_DP_CONTROLLER_AUX_OFFSET;
-	return readl_relaxed(catalog->io->dp_controller.base + offset);
+	return readl_relaxed(catalog->io->dp_controller.aux + offset);
 }
 
 static inline void dp_write_aux(struct dp_catalog_private *catalog,
 			       u32 offset, u32 data)
 {
-	offset += MSM_DP_CONTROLLER_AUX_OFFSET;
 	/*
 	 * To make sure aux reg writes happens before any other operation,
 	 * this function uses writel() instread of writel_relaxed()
 	 */
-	writel(data, catalog->io->dp_controller.base + offset);
+	writel(data, catalog->io->dp_controller.aux + offset);
 }
 
 static inline u32 dp_read_ahb(struct dp_catalog_private *catalog, u32 offset)
 {
-	offset += MSM_DP_CONTROLLER_AHB_OFFSET;
-	return readl_relaxed(catalog->io->dp_controller.base + offset);
+	return readl_relaxed(catalog->io->dp_controller.ahb + offset);
 }
 
 static inline void dp_write_ahb(struct dp_catalog_private *catalog,
 			       u32 offset, u32 data)
 {
-	offset += MSM_DP_CONTROLLER_AHB_OFFSET;
 	/*
 	 * To make sure phy reg writes happens before any other operation,
 	 * this function uses writel() instread of writel_relaxed()
 	 */
-	writel(data, catalog->io->dp_controller.base + offset);
+	writel(data, catalog->io->dp_controller.ahb + offset);
 }
 
 static inline void dp_write_p0(struct dp_catalog_private *catalog,
 			       u32 offset, u32 data)
 {
-	offset += MSM_DP_CONTROLLER_P0_OFFSET;
 	/*
 	 * To make sure interface reg writes happens before any other operation,
 	 * this function uses writel() instread of writel_relaxed()
 	 */
-	writel(data, catalog->io->dp_controller.base + offset);
+	writel(data, catalog->io->dp_controller.p0 + offset);
 }
 
 static inline u32 dp_read_p0(struct dp_catalog_private *catalog,
 			       u32 offset)
 {
-	offset += MSM_DP_CONTROLLER_P0_OFFSET;
 	/*
 	 * To make sure interface reg writes happens before any other operation,
 	 * this function uses writel() instread of writel_relaxed()
 	 */
-	return readl_relaxed(catalog->io->dp_controller.base + offset);
+	return readl_relaxed(catalog->io->dp_controller.p0 + offset);
 }
 
 static inline u32 dp_read_link(struct dp_catalog_private *catalog, u32 offset)
 {
-	offset += MSM_DP_CONTROLLER_LINK_OFFSET;
-	return readl_relaxed(catalog->io->dp_controller.base + offset);
+	return readl_relaxed(catalog->io->dp_controller.link + offset);
 }
 
 static inline void dp_write_link(struct dp_catalog_private *catalog,
 			       u32 offset, u32 data)
 {
-	offset += MSM_DP_CONTROLLER_LINK_OFFSET;
 	/*
 	 * To make sure link reg writes happens before any other operation,
 	 * this function uses writel() instread of writel_relaxed()
 	 */
-	writel(data, catalog->io->dp_controller.base + offset);
+	writel(data, catalog->io->dp_controller.link + offset);
 }
 
 /* aux related catalog functions */
@@ -267,29 +250,21 @@ static void dump_regs(void __iomem *base, int len)
 
 void dp_catalog_dump_regs(struct dp_catalog *dp_catalog)
 {
-	u32 offset, len;
 	struct dp_catalog_private *catalog = container_of(dp_catalog,
 		struct dp_catalog_private, dp_catalog);
+	struct dss_io_data *io = &catalog->io->dp_controller;
 
 	pr_info("AHB regs\n");
-	offset = MSM_DP_CONTROLLER_AHB_OFFSET;
-	len = MSM_DP_CONTROLLER_AHB_SIZE;
-	dump_regs(catalog->io->dp_controller.base + offset, len);
+	dump_regs(io->ahb, io->ahb_len);
 
 	pr_info("AUXCLK regs\n");
-	offset = MSM_DP_CONTROLLER_AUX_OFFSET;
-	len = MSM_DP_CONTROLLER_AUX_SIZE;
-	dump_regs(catalog->io->dp_controller.base + offset, len);
+	dump_regs(io->aux, io->aux_len);
 
 	pr_info("LCLK regs\n");
-	offset = MSM_DP_CONTROLLER_LINK_OFFSET;
-	len = MSM_DP_CONTROLLER_LINK_SIZE;
-	dump_regs(catalog->io->dp_controller.base + offset, len);
+	dump_regs(io->link, io->link_len);
 
 	pr_info("P0CLK regs\n");
-	offset = MSM_DP_CONTROLLER_P0_OFFSET;
-	len = MSM_DP_CONTROLLER_P0_SIZE;
-	dump_regs(catalog->io->dp_controller.base + offset, len);
+	dump_regs(io->p0, io->p0_len);
 }
 
 int dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog)
@@ -454,8 +429,7 @@ int dp_catalog_ctrl_set_pattern(struct dp_catalog *dp_catalog,
 	bit = BIT(pattern - 1) << DP_MAINLINK_READY_LINK_TRAINING_SHIFT;
 
 	/* Poll for mainlink ready status */
-	ret = readx_poll_timeout(readl, catalog->io->dp_controller.base +
-					MSM_DP_CONTROLLER_LINK_OFFSET +
+	ret = readx_poll_timeout(readl, catalog->io->dp_controller.link +
 					REG_DP_MAINLINK_READY,
 					data, data & bit,
 					POLLING_SLEEP_US, POLLING_TIMEOUT_US);
@@ -502,8 +476,7 @@ bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog *dp_catalog)
 				struct dp_catalog_private, dp_catalog);
 
 	/* Poll for mainlink ready status */
-	ret = readl_poll_timeout(catalog->io->dp_controller.base +
-				MSM_DP_CONTROLLER_LINK_OFFSET +
+	ret = readl_poll_timeout(catalog->io->dp_controller.link +
 				REG_DP_MAINLINK_READY,
 				data, data & DP_MAINLINK_READY_FOR_VIDEO,
 				POLLING_SLEEP_US, POLLING_TIMEOUT_US);
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index 0519dd3ac3c3..51ec85b4803b 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -250,6 +250,7 @@ static int dp_parser_clock(struct dp_parser *parser)
 
 static int dp_parser_parse(struct dp_parser *parser)
 {
+	struct dss_io_data *io = &parser->io.dp_controller;
 	int rc = 0;
 
 	if (!parser) {
@@ -275,6 +276,15 @@ static int dp_parser_parse(struct dp_parser *parser)
 	 */
 	parser->regulator_cfg = &sdm845_dp_reg_cfg;
 
+	io->ahb = io->base + 0x0;
+	io->ahb_len = 0x200;
+	io->aux = io->base + 0x200;
+	io->aux_len = 0x200;
+	io->link = io->base + 0x400;
+	io->link_len = 0x600;
+	io->p0 = io->base + 0x1000;
+	io->p0_len = 0x400;
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
index 34b49628bbaf..ff4774109c63 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -28,6 +28,14 @@ enum dp_pm_type {
 struct dss_io_data {
 	u32 len;
 	void __iomem *base;
+	void __iomem *ahb;
+	size_t ahb_len;
+	void __iomem *aux;
+	size_t aux_len;
+	void __iomem *link;
+	size_t link_len;
+	void __iomem *p0;
+	size_t p0_len;
 };
 
 static inline const char *dp_parser_pm_name(enum dp_pm_type module)
-- 
2.29.2

[PATCH 3/4] drm/msm/dp: Initialize the INTF_CONFIG register

[Bjorn Andersson]

Some bootloaders set the widebus enable bit in the INTF_CONFIG register,
but configuration of widebus isn't yet supported ensure that the
register has a known value, with widebus disabled.

Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_catalog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index a0449a2867e4..e3996eef5518 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -707,6 +707,7 @@ int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog)
 	dp_write_link(catalog, REG_DP_HSYNC_VSYNC_WIDTH_POLARITY,
 				dp_catalog->width_blanking);
 	dp_write_link(catalog, REG_DP_ACTIVE_HOR_VER, dp_catalog->dp_active);
+	dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, 0);
 	return 0;
 }
 
-- 
2.29.2

[PATCH 4/4] drm/msm/dp: Add support for SC8180x eDP

[Bjorn Andersson]

The eDP controller found in SC8180x is at large compatible with the
current implementation, but has its register blocks at slightly
different offsets.

Add the compatible and the new register layout.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c |  1 +
 drivers/gpu/drm/msm/dp/dp_parser.c  | 28 ++++++++++++++++++++--------
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index d1319b58e901..0be03bdc882c 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -121,6 +121,7 @@ struct dp_display_private {
 
 static const struct of_device_id dp_dt_match[] = {
 	{.compatible = "qcom,sc7180-dp"},
+	{ .compatible = "qcom,sc8180x-edp" },
 	{}
 };
 
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index 51ec85b4803b..47cf18bba4b2 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -251,6 +251,7 @@ static int dp_parser_clock(struct dp_parser *parser)
 static int dp_parser_parse(struct dp_parser *parser)
 {
 	struct dss_io_data *io = &parser->io.dp_controller;
+	struct device *dev = &parser->pdev->dev;
 	int rc = 0;
 
 	if (!parser) {
@@ -276,14 +277,25 @@ static int dp_parser_parse(struct dp_parser *parser)
 	 */
 	parser->regulator_cfg = &sdm845_dp_reg_cfg;
 
-	io->ahb = io->base + 0x0;
-	io->ahb_len = 0x200;
-	io->aux = io->base + 0x200;
-	io->aux_len = 0x200;
-	io->link = io->base + 0x400;
-	io->link_len = 0x600;
-	io->p0 = io->base + 0x1000;
-	io->p0_len = 0x400;
+	if (of_device_is_compatible(dev->of_node, "qcom,sc8180x-edp")) {
+		io->ahb = io->base + 0x0;
+		io->ahb_len = 0x200;
+		io->aux = io->base + 0x200;
+		io->aux_len = 0x200;
+		io->link = io->base + 0x400;
+		io->link_len = 0x600;
+		io->p0 = io->base + 0xa00;
+		io->p0_len = 0x400;
+	} else {
+		io->ahb = io->base + 0x0;
+		io->ahb_len = 0x200;
+		io->aux = io->base + 0x200;
+		io->aux_len = 0x200;
+		io->link = io->base + 0x400;
+		io->link_len = 0x600;
+		io->p0 = io->base + 0x1000;
+		io->p0_len = 0x400;
+	}
 
 	return 0;
 }
-- 
2.29.2

Re: [Freedreno] [PATCH 0/4] drm/msm/dp: Add support for SC8180x eDP controller

[abhinavk]

Hi Bjorn

I had a quick glance on the series and before getting to other things 
wanted to know how you are initializing two different connectors for
DP & EDP resp.

The connector type for DP should be DRM_MODE_CONNECTOR_DisplayPort and 
eDP should be DRM_MODE_CONNECTOR_eDP.
We need both to be created so that both EDP and DP can be supported 
concurrently.

Will these changes work for concurrent eDP and DP case?

Thanks

Abhinav

On 2021-05-10 21:20, Bjorn Andersson wrote:
> The first patch in the series is somewhat unrelated to the support, but
> simplifies reasoning and debugging of timing related issues.
> 
> The second patch introduces support for dealing with different register 
> block
> layouts, which is used in the forth patch to describe the hardware 
> blocks found
> in the SC8180x eDP block.
> 
> The third patch configures the INTF_CONFIG register, which carries the
> configuration for widebus handling. As with the DPU the bootloader 
> enables
> widebus and we need to disable it, or implement support for adjusting 
> the
> timing.
> 
> Bjorn Andersson (4):
>   drm/msm/dp: Simplify the mvid/nvid calculation
>   drm/msm/dp: Store each subblock in the io region
>   drm/msm/dp: Initialize the INTF_CONFIG register
>   drm/msm/dp: Add support for SC8180x eDP
> 
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 99 +++++++----------------------
>  drivers/gpu/drm/msm/dp/dp_display.c |  1 +
>  drivers/gpu/drm/msm/dp/dp_parser.c  | 22 +++++++
>  drivers/gpu/drm/msm/dp/dp_parser.h  |  8 +++
>  4 files changed, 53 insertions(+), 77 deletions(-)

Re: [Freedreno] [PATCH 0/4] drm/msm/dp: Add support for SC8180x eDP controller

[Bjorn Andersson]

On Tue 18 May 22:41 CDT 2021, abhinavk@codeaurora.org wrote:

> Hi Bjorn
> 
> I had a quick glance on the series and before getting to other things wanted
> to know how you are initializing two different connectors for
> DP & EDP resp.
> 
> The connector type for DP should be DRM_MODE_CONNECTOR_DisplayPort and eDP
> should be DRM_MODE_CONNECTOR_eDP.

As far as I've been able to conclude there is no eDP support in the
upstream DPU driver; an encoder of type DRM_MODE_ENCODER_TMDS will only
attach to INTF_DP.

> We need both to be created so that both EDP and DP can be supported
> concurrently.
> 

Further more the DP controller driver has a global variable to track
state and the INTF-picker will always pick the interface of index 0 when
setting up the DP controller.

> Will these changes work for concurrent eDP and DP case?
> 

The proposed changes are all that I need to get eDP working on my
sc8180x laptop. But the DPU code does not currently support more than a
single DP interface - and that has to be on the first INTF_DP that the
DPU driver knows about.

But this is a limitation we should fix, rather than claiming that you
can only have one of each. Further more, afaict the sc7280 DP controller
can do both DP and eDP, so it would make sense not to distinguish the
interfaces as eDP or DP - just because the product in mind will use eDP.


PS. I've currently disabled the eDP interface on my laptop and am
working on trying to get Type-C DP working. Once that's in place I'd
need a better INTF/encoder picker - because the current model of just
picking INTF_DP 0 (or in a sequential fashion) won't work.

Regards,
Bjorn

> Thanks
> 
> Abhinav
> 
> On 2021-05-10 21:20, Bjorn Andersson wrote:
> > The first patch in the series is somewhat unrelated to the support, but
> > simplifies reasoning and debugging of timing related issues.
> > 
> > The second patch introduces support for dealing with different register
> > block
> > layouts, which is used in the forth patch to describe the hardware
> > blocks found
> > in the SC8180x eDP block.
> > 
> > The third patch configures the INTF_CONFIG register, which carries the
> > configuration for widebus handling. As with the DPU the bootloader
> > enables
> > widebus and we need to disable it, or implement support for adjusting
> > the
> > timing.
> > 
> > Bjorn Andersson (4):
> >   drm/msm/dp: Simplify the mvid/nvid calculation
> >   drm/msm/dp: Store each subblock in the io region
> >   drm/msm/dp: Initialize the INTF_CONFIG register
> >   drm/msm/dp: Add support for SC8180x eDP
> > 
> >  drivers/gpu/drm/msm/dp/dp_catalog.c | 99 +++++++----------------------
> >  drivers/gpu/drm/msm/dp/dp_display.c |  1 +
> >  drivers/gpu/drm/msm/dp/dp_parser.c  | 22 +++++++
> >  drivers/gpu/drm/msm/dp/dp_parser.h  |  8 +++
> >  4 files changed, 53 insertions(+), 77 deletions(-)

Re: [Freedreno] [PATCH 0/4] drm/msm/dp: Add support for SC8180x eDP controller

[abhinavk]

Hi Bjorn

On 2021-05-19 07:51, Bjorn Andersson wrote:
> On Tue 18 May 22:41 CDT 2021, abhinavk@codeaurora.org wrote:
> 
>> Hi Bjorn
>> 
>> I had a quick glance on the series and before getting to other things 
>> wanted
>> to know how you are initializing two different connectors for
>> DP & EDP resp.
>> 
>> The connector type for DP should be DRM_MODE_CONNECTOR_DisplayPort and 
>> eDP
>> should be DRM_MODE_CONNECTOR_eDP.
> 
> As far as I've been able to conclude there is no eDP support in the
> upstream DPU driver; an encoder of type DRM_MODE_ENCODER_TMDS will only
> attach to INTF_DP.
> 
>> We need both to be created so that both EDP and DP can be supported
>> concurrently.
>> 
> 
> Further more the DP controller driver has a global variable to track
> state and the INTF-picker will always pick the interface of index 0 
> when
> setting up the DP controller.
> 
>> Will these changes work for concurrent eDP and DP case?
>> 
> 
> The proposed changes are all that I need to get eDP working on my
> sc8180x laptop. But the DPU code does not currently support more than a
> single DP interface - and that has to be on the first INTF_DP that the
> DPU driver knows about.
> 
> But this is a limitation we should fix, rather than claiming that you
> can only have one of each. Further more, afaict the sc7280 DP 
> controller
> can do both DP and eDP, so it would make sense not to distinguish the
> interfaces as eDP or DP - just because the product in mind will use 
> eDP.
> 
> 
> PS. I've currently disabled the eDP interface on my laptop and am
> working on trying to get Type-C DP working. Once that's in place I'd
> need a better INTF/encoder picker - because the current model of just
> picking INTF_DP 0 (or in a sequential fashion) won't work.
> 
> Regards,
> Bjorn

Yes, we should be able to support eDP + DP concurrently on both sc7280
and sc8180x. Some of the changes we will be posting in the coming weeks
should add support for it. Till then, as we spoke on IRC, since your 
changes
dont break existing DP functionality, will continue reviewing rest of 
the changes.

> 
>> Thanks
>> 
>> Abhinav
>> 
>> On 2021-05-10 21:20, Bjorn Andersson wrote:
>> > The first patch in the series is somewhat unrelated to the support, but
>> > simplifies reasoning and debugging of timing related issues.
>> >
>> > The second patch introduces support for dealing with different register
>> > block
>> > layouts, which is used in the forth patch to describe the hardware
>> > blocks found
>> > in the SC8180x eDP block.
>> >
>> > The third patch configures the INTF_CONFIG register, which carries the
>> > configuration for widebus handling. As with the DPU the bootloader
>> > enables
>> > widebus and we need to disable it, or implement support for adjusting
>> > the
>> > timing.
>> >
>> > Bjorn Andersson (4):
>> >   drm/msm/dp: Simplify the mvid/nvid calculation
>> >   drm/msm/dp: Store each subblock in the io region
>> >   drm/msm/dp: Initialize the INTF_CONFIG register
>> >   drm/msm/dp: Add support for SC8180x eDP
>> >
>> >  drivers/gpu/drm/msm/dp/dp_catalog.c | 99 +++++++----------------------
>> >  drivers/gpu/drm/msm/dp/dp_display.c |  1 +
>> >  drivers/gpu/drm/msm/dp/dp_parser.c  | 22 +++++++
>> >  drivers/gpu/drm/msm/dp/dp_parser.h  |  8 +++
>> >  4 files changed, 53 insertions(+), 77 deletions(-)

Re: [Freedreno] [PATCH 1/4] drm/msm/dp: Simplify the mvid/nvid calculation

[abhinavk]

Hi Bjorn

On 2021-05-10 21:20, Bjorn Andersson wrote:
> In the search for causes to timing issues seen during implementation of
> eDP support for SC8180x a fair amount of time was spent concluding why
> the calculated mvid/nvid values where wrong.
> 
> The overall conclusion is that the ratio of MVID/NVID describes, and
> should match, the ratio between the pixel and link clock.
> 
> Downstream this calculation reads the M and N values off the pixel 
> clock
> straight from DISP_CC and are then adjusted based on knowledge of how
> the link and vco_div (parent of the pixel clock) are derrived from the
> common VCO.
> 
> While upstreaming, and then extracting the PHY driver, the resulting
> function performs the following steps:
> 
> 1) Adjust the passed link rate based on the VCO divider used in the PHY
>    driver, and multiply this by 10 based on the link rate divider.
> 2) Pick reasonable choices of M and N, by calculating the ratio between
>    this new clock and the pixel clock.
> 3) Subtract M from N and flip the bits, to match the encoding of the N
>    register in DISP_CC.
> 4) Flip the bits of N and add M, to get the value of N back.
> 5) Multiply M with 5, per the documentation.
> 6) Scale the values such that N is close to 0x8000 (or larger)
> 7) Multply M with 2 or 3 depending on the link rate of HBR2 or HBR3.
> 
> Presumably step 3) was added to provide step 4) with expected input, so
> the two cancel each other out. The factor of 10 from step 1) goes into
> the denominator and is partially cancelled by the 5 in the numerator in
> step 5), resulting in step 7) simply cancelling out step 1).
> 

Both the multiplication of M with 5 and N with 2 or 3 is coming because 
of the
ratio between the vco clk and the link clk.
So we could have 2.7, 5.4 or 8.1 Gbps link clks and the factor of 2 or 3
gets added because hbr2 is 2 * hbr and hbr3 is 3 * hbr.

Your summary is pretty much right otherwise. Let me add some more points 
here:

1) Originally we removed reading the M_VID and N_VID from the DISPCC 
regs because
of previous upstream comments that we can potentially just recalculate 
whatever the clk driver is programming
by using rational_best_approximation
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/clk/qcom/clk-rcg2.c#L1160

Not having to read from DISPCC register is also useful because we dont 
have to maintain the register offset
of the M_VID and N_VID which keeps changing across chipsets.

However we discussed this again after viewing this patch. So the clk 
driver always operates on the vco clk
and calculates the pixel clk from it and sets the M_VID and N_VID based 
on that.
In terms of accuracy, the best way is still to re-use the M_VID and 
N_VID which the clk driver sets because
the pixel clock was generated based on that and that is the actual pixel 
clock we are going to get.

So even before this change we lost some accuracy because the pixel clock 
we are giving here to recalculate
the M_VID and N_VID is a theoretical value. Although for most values of 
pixel clk, theoretical and actual
should match. There could be corner cases of pixel clock where its a bit 
different. Hence ideally, re-using the M_VID
and N_VID which the clk driver set would have been the best but not 
having to hard-code M_VID and N_VID offsets
was a good enough reason to not go back to that again.

Now, coming to this change. Here its trying to again re-calculate the 
M_VID and N_VID by using the same
API which the clk driver uses but uses link clk and pixel clk as the 
parameters Vs the clk driver uses
vco clk and actual pixel clock to calculate this.

So even though this cleanup eliminates the adjustments we need to make 
to account for the VCO clk to link clk ratio,
it also could bring additional difference between what was actually set 
by the clk driver and what we are calculating
here because clk driver used vco clk as the input vs here we use link 
clk after this change.
There might be some pixel clock rates of some resolutions where this 
difference could be risky.

Hence the overall conclusion here was to keep using vco clk as the input 
to rational_best_approximation
and not make more changes to this.

> Left is the code that finds the ratio between the two arguments, scaled
> to keep the denominator close to or larger than 0x8000. And this is our
> mvid/nvid pair.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 41 +++++------------------------
>  1 file changed, 6 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index b1a9b1b98f5f..2eb37ee48e42 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -415,39 +415,16 @@ void dp_catalog_ctrl_config_msa(struct
> dp_catalog *dp_catalog,
>  					u32 rate, u32 stream_rate_khz,
>  					bool fixed_nvid)
>  {
> -	u32 pixel_m, pixel_n;
> -	u32 mvid, nvid, pixel_div = 0, dispcc_input_rate;
>  	u32 const nvid_fixed = DP_LINK_CONSTANT_N_VALUE;
> -	u32 const link_rate_hbr2 = 540000;
> -	u32 const link_rate_hbr3 = 810000;
> -	unsigned long den, num;
> -
> +	unsigned long mvid, nvid;
>  	struct dp_catalog_private *catalog = container_of(dp_catalog,
>  				struct dp_catalog_private, dp_catalog);
> 
> -	if (rate == link_rate_hbr3)
> -		pixel_div = 6;
> -	else if (rate == 1620000 || rate == 270000)
> -		pixel_div = 2;
> -	else if (rate == link_rate_hbr2)
> -		pixel_div = 4;
> -	else
> -		DRM_ERROR("Invalid pixel mux divider\n");
> -
> -	dispcc_input_rate = (rate * 10) / pixel_div;
> -
> -	rational_best_approximation(dispcc_input_rate, stream_rate_khz,
> -			(unsigned long)(1 << 16) - 1,
> -			(unsigned long)(1 << 16) - 1, &den, &num);
> -
> -	den = ~(den - num);
> -	den = den & 0xFFFF;
> -	pixel_m = num;
> -	pixel_n = den;
> -
> -	mvid = (pixel_m & 0xFFFF) * 5;
> -	nvid = (0xFFFF & (~pixel_n)) + (pixel_m & 0xFFFF);
> +	rational_best_approximation(stream_rate_khz, rate,
> +				    (1 << 16) - 1, (1 << 16) - 1,
> +				    &mvid, &nvid);
> 
> +	/* Adjust values so that nvid is close to DP_LINK_CONSTANT_N_VALUE */
>  	if (nvid < nvid_fixed) {
>  		u32 temp;
> 
> @@ -456,13 +433,7 @@ void dp_catalog_ctrl_config_msa(struct dp_catalog
> *dp_catalog,
>  		nvid = temp;
>  	}
> 
> -	if (link_rate_hbr2 == rate)
> -		nvid *= 2;
> -
> -	if (link_rate_hbr3 == rate)
> -		nvid *= 3;
> -
> -	DRM_DEBUG_DP("mvid=0x%x, nvid=0x%x\n", mvid, nvid);
> +	DRM_DEBUG_DP("mvid=0x%lx, nvid=0x%lx\n", mvid, nvid);
>  	dp_write_link(catalog, REG_DP_SOFTWARE_MVID, mvid);
>  	dp_write_link(catalog, REG_DP_SOFTWARE_NVID, nvid);
>  	dp_write_p0(catalog, MMSS_DP_DSC_DTO, 0x0);

Re: [Freedreno] [PATCH 2/4] drm/msm/dp: Store each subblock in the io region

[abhinavk]

On 2021-05-10 21:20, Bjorn Andersson wrote:
> Not all platforms has DP_P0 at offset 0x1000 from the beginning of the
> DP block. So move the offsets into dss_io_data, to make it possible in
> the next patch to specify alternative offsets and sizes of these
> segments.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 57 ++++++++---------------------
>  drivers/gpu/drm/msm/dp/dp_parser.c  | 10 +++++
>  drivers/gpu/drm/msm/dp/dp_parser.h  |  8 ++++
>  3 files changed, 33 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 2eb37ee48e42..a0449a2867e4 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -24,15 +24,6 @@
>  #define DP_INTERRUPT_STATUS_ACK_SHIFT	1
>  #define DP_INTERRUPT_STATUS_MASK_SHIFT	2
> 
> -#define MSM_DP_CONTROLLER_AHB_OFFSET	0x0000
> -#define MSM_DP_CONTROLLER_AHB_SIZE	0x0200
> -#define MSM_DP_CONTROLLER_AUX_OFFSET	0x0200
> -#define MSM_DP_CONTROLLER_AUX_SIZE	0x0200
> -#define MSM_DP_CONTROLLER_LINK_OFFSET	0x0400
> -#define MSM_DP_CONTROLLER_LINK_SIZE	0x0C00
> -#define MSM_DP_CONTROLLER_P0_OFFSET	0x1000
> -#define MSM_DP_CONTROLLER_P0_SIZE	0x0400
> -
>  #define DP_INTERRUPT_STATUS1 \
>  	(DP_INTR_AUX_I2C_DONE| \
>  	DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \
> @@ -64,75 +55,67 @@ struct dp_catalog_private {
> 
>  static inline u32 dp_read_aux(struct dp_catalog_private *catalog, u32 
> offset)
>  {
> -	offset += MSM_DP_CONTROLLER_AUX_OFFSET;
> -	return readl_relaxed(catalog->io->dp_controller.base + offset);
> +	return readl_relaxed(catalog->io->dp_controller.aux + offset);
>  }
> 
>  static inline void dp_write_aux(struct dp_catalog_private *catalog,
>  			       u32 offset, u32 data)
>  {
> -	offset += MSM_DP_CONTROLLER_AUX_OFFSET;
>  	/*
>  	 * To make sure aux reg writes happens before any other operation,
>  	 * this function uses writel() instread of writel_relaxed()
>  	 */
> -	writel(data, catalog->io->dp_controller.base + offset);
> +	writel(data, catalog->io->dp_controller.aux + offset);
>  }
> 
>  static inline u32 dp_read_ahb(struct dp_catalog_private *catalog, u32 
> offset)
>  {
> -	offset += MSM_DP_CONTROLLER_AHB_OFFSET;
> -	return readl_relaxed(catalog->io->dp_controller.base + offset);
> +	return readl_relaxed(catalog->io->dp_controller.ahb + offset);
>  }
> 
>  static inline void dp_write_ahb(struct dp_catalog_private *catalog,
>  			       u32 offset, u32 data)
>  {
> -	offset += MSM_DP_CONTROLLER_AHB_OFFSET;
>  	/*
>  	 * To make sure phy reg writes happens before any other operation,
>  	 * this function uses writel() instread of writel_relaxed()
>  	 */
> -	writel(data, catalog->io->dp_controller.base + offset);
> +	writel(data, catalog->io->dp_controller.ahb + offset);
>  }
> 
>  static inline void dp_write_p0(struct dp_catalog_private *catalog,
>  			       u32 offset, u32 data)
>  {
> -	offset += MSM_DP_CONTROLLER_P0_OFFSET;
>  	/*
>  	 * To make sure interface reg writes happens before any other 
> operation,
>  	 * this function uses writel() instread of writel_relaxed()
>  	 */
> -	writel(data, catalog->io->dp_controller.base + offset);
> +	writel(data, catalog->io->dp_controller.p0 + offset);
>  }
> 
>  static inline u32 dp_read_p0(struct dp_catalog_private *catalog,
>  			       u32 offset)
>  {
> -	offset += MSM_DP_CONTROLLER_P0_OFFSET;
>  	/*
>  	 * To make sure interface reg writes happens before any other 
> operation,
>  	 * this function uses writel() instread of writel_relaxed()
>  	 */
> -	return readl_relaxed(catalog->io->dp_controller.base + offset);
> +	return readl_relaxed(catalog->io->dp_controller.p0 + offset);
>  }
> 
>  static inline u32 dp_read_link(struct dp_catalog_private *catalog, u32 
> offset)
>  {
> -	offset += MSM_DP_CONTROLLER_LINK_OFFSET;
> -	return readl_relaxed(catalog->io->dp_controller.base + offset);
> +	return readl_relaxed(catalog->io->dp_controller.link + offset);
>  }
> 
>  static inline void dp_write_link(struct dp_catalog_private *catalog,
>  			       u32 offset, u32 data)
>  {
> -	offset += MSM_DP_CONTROLLER_LINK_OFFSET;
>  	/*
>  	 * To make sure link reg writes happens before any other operation,
>  	 * this function uses writel() instread of writel_relaxed()
>  	 */
> -	writel(data, catalog->io->dp_controller.base + offset);
> +	writel(data, catalog->io->dp_controller.link + offset);
>  }
> 
>  /* aux related catalog functions */
> @@ -267,29 +250,21 @@ static void dump_regs(void __iomem *base, int 
> len)
> 
>  void dp_catalog_dump_regs(struct dp_catalog *dp_catalog)
>  {
> -	u32 offset, len;
>  	struct dp_catalog_private *catalog = container_of(dp_catalog,
>  		struct dp_catalog_private, dp_catalog);
> +	struct dss_io_data *io = &catalog->io->dp_controller;
> 
>  	pr_info("AHB regs\n");
> -	offset = MSM_DP_CONTROLLER_AHB_OFFSET;
> -	len = MSM_DP_CONTROLLER_AHB_SIZE;
> -	dump_regs(catalog->io->dp_controller.base + offset, len);
> +	dump_regs(io->ahb, io->ahb_len);
> 
>  	pr_info("AUXCLK regs\n");
> -	offset = MSM_DP_CONTROLLER_AUX_OFFSET;
> -	len = MSM_DP_CONTROLLER_AUX_SIZE;
> -	dump_regs(catalog->io->dp_controller.base + offset, len);
> +	dump_regs(io->aux, io->aux_len);
> 
>  	pr_info("LCLK regs\n");
> -	offset = MSM_DP_CONTROLLER_LINK_OFFSET;
> -	len = MSM_DP_CONTROLLER_LINK_SIZE;
> -	dump_regs(catalog->io->dp_controller.base + offset, len);
> +	dump_regs(io->link, io->link_len);
> 
>  	pr_info("P0CLK regs\n");
> -	offset = MSM_DP_CONTROLLER_P0_OFFSET;
> -	len = MSM_DP_CONTROLLER_P0_SIZE;
> -	dump_regs(catalog->io->dp_controller.base + offset, len);
> +	dump_regs(io->p0, io->p0_len);
>  }
> 
>  int dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog)
> @@ -454,8 +429,7 @@ int dp_catalog_ctrl_set_pattern(struct dp_catalog
> *dp_catalog,
>  	bit = BIT(pattern - 1) << DP_MAINLINK_READY_LINK_TRAINING_SHIFT;
> 
>  	/* Poll for mainlink ready status */
> -	ret = readx_poll_timeout(readl, catalog->io->dp_controller.base +
> -					MSM_DP_CONTROLLER_LINK_OFFSET +
> +	ret = readx_poll_timeout(readl, catalog->io->dp_controller.link +
>  					REG_DP_MAINLINK_READY,
>  					data, data & bit,
>  					POLLING_SLEEP_US, POLLING_TIMEOUT_US);
> @@ -502,8 +476,7 @@ bool dp_catalog_ctrl_mainlink_ready(struct
> dp_catalog *dp_catalog)
>  				struct dp_catalog_private, dp_catalog);
> 
>  	/* Poll for mainlink ready status */
> -	ret = readl_poll_timeout(catalog->io->dp_controller.base +
> -				MSM_DP_CONTROLLER_LINK_OFFSET +
> +	ret = readl_poll_timeout(catalog->io->dp_controller.link +
>  				REG_DP_MAINLINK_READY,
>  				data, data & DP_MAINLINK_READY_FOR_VIDEO,
>  				POLLING_SLEEP_US, POLLING_TIMEOUT_US);
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> b/drivers/gpu/drm/msm/dp/dp_parser.c
> index 0519dd3ac3c3..51ec85b4803b 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -250,6 +250,7 @@ static int dp_parser_clock(struct dp_parser 
> *parser)
> 
>  static int dp_parser_parse(struct dp_parser *parser)
>  {
> +	struct dss_io_data *io = &parser->io.dp_controller;
>  	int rc = 0;
> 
>  	if (!parser) {
> @@ -275,6 +276,15 @@ static int dp_parser_parse(struct dp_parser 
> *parser)
>  	 */
>  	parser->regulator_cfg = &sdm845_dp_reg_cfg;
> 
> +	io->ahb = io->base + 0x0;
> +	io->ahb_len = 0x200;
> +	io->aux = io->base + 0x200;
> +	io->aux_len = 0x200;
> +	io->link = io->base + 0x400;
> +	io->link_len = 0x600;
> +	io->p0 = io->base + 0x1000;
> +	io->p0_len = 0x400;
> +
>  	return 0;
>  }
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h
> b/drivers/gpu/drm/msm/dp/dp_parser.h
> index 34b49628bbaf..ff4774109c63 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -28,6 +28,14 @@ enum dp_pm_type {
>  struct dss_io_data {
>  	u32 len;
>  	void __iomem *base;
> +	void __iomem *ahb;
> +	size_t ahb_len;
> +	void __iomem *aux;
> +	size_t aux_len;
> +	void __iomem *link;
> +	size_t link_len;
> +	void __iomem *p0;
> +	size_t p0_len;
>  };
> 
>  static inline const char *dp_parser_pm_name(enum dp_pm_type module)

Re: [Freedreno] [PATCH 3/4] drm/msm/dp: Initialize the INTF_CONFIG register

[abhinavk]

On 2021-05-10 21:20, Bjorn Andersson wrote:
> Some bootloaders set the widebus enable bit in the INTF_CONFIG 
> register,
> but configuration of widebus isn't yet supported ensure that the
> register has a known value, with widebus disabled.
> 
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index a0449a2867e4..e3996eef5518 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -707,6 +707,7 @@ int dp_catalog_panel_timing_cfg(struct dp_catalog
> *dp_catalog)
>  	dp_write_link(catalog, REG_DP_HSYNC_VSYNC_WIDTH_POLARITY,
>  				dp_catalog->width_blanking);
>  	dp_write_link(catalog, REG_DP_ACTIVE_HOR_VER, dp_catalog->dp_active);
> +	dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, 0);
>  	return 0;
>  }

Re: [Freedreno] [PATCH 4/4] drm/msm/dp: Add support for SC8180x eDP

[abhinavk]

On 2021-05-10 21:20, Bjorn Andersson wrote:
> The eDP controller found in SC8180x is at large compatible with the
> current implementation, but has its register blocks at slightly
> different offsets.
> 
> Add the compatible and the new register layout.
> 
I am not able to completely recall the history of why in the DP bindings
we added DP register base as a big hunk and let catalog handle the 
submodule
offsets.

I guess earlier that made sense because DP sub-block offsets were fixed.
But if we plan to re-use the DP driver for eDP as well like this series, 
then maybe it might be
better if this comes from device tree like the earlier version was 
planning to

https://patchwork.kernel.org/project/dri-devel/patch/0101016ec6ddf446-e87ab1ce-5cbf-40a0-a0bb-cd0151cd577a-000000@us-west-2.amazonses.com/


+- reg:                  Base address and length of DP hardware's memory 
mapped regions.
+- cell-index:           Specifies the controller instance.
+- reg-names:            A list of strings that name the list of regs.
+			"dp_ahb" - DP controller memory region.
+			"dp_aux" - DP AUX memory region.
+			"dp_link" - DP link layer memory region.
+			"dp_p0" - DP pixel clock domain memory region.
+			"dp_phy" - DP PHY memory region.
+			"dp_ln_tx0" - USB3 DP PHY combo TX-0 lane memory region.
+			"dp_ln_tx1" - USB3 DP PHY combo TX-1 lane memory region.

Now there is more reason to separate the sub-module offsets like 
ahb/aux/link/p0
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c |  1 +
>  drivers/gpu/drm/msm/dp/dp_parser.c  | 28 ++++++++++++++++++++--------
>  2 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index d1319b58e901..0be03bdc882c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -121,6 +121,7 @@ struct dp_display_private {
> 
>  static const struct of_device_id dp_dt_match[] = {
>  	{.compatible = "qcom,sc7180-dp"},
> +	{ .compatible = "qcom,sc8180x-edp" },
>  	{}
>  };
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> b/drivers/gpu/drm/msm/dp/dp_parser.c
> index 51ec85b4803b..47cf18bba4b2 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -251,6 +251,7 @@ static int dp_parser_clock(struct dp_parser 
> *parser)
>  static int dp_parser_parse(struct dp_parser *parser)
>  {
>  	struct dss_io_data *io = &parser->io.dp_controller;
> +	struct device *dev = &parser->pdev->dev;
>  	int rc = 0;
> 
>  	if (!parser) {
> @@ -276,14 +277,25 @@ static int dp_parser_parse(struct dp_parser 
> *parser)
>  	 */
>  	parser->regulator_cfg = &sdm845_dp_reg_cfg;
> 
> -	io->ahb = io->base + 0x0;
> -	io->ahb_len = 0x200;
> -	io->aux = io->base + 0x200;
> -	io->aux_len = 0x200;
> -	io->link = io->base + 0x400;
> -	io->link_len = 0x600;
> -	io->p0 = io->base + 0x1000;
> -	io->p0_len = 0x400;
> +	if (of_device_is_compatible(dev->of_node, "qcom,sc8180x-edp")) {
> +		io->ahb = io->base + 0x0;
> +		io->ahb_len = 0x200;
> +		io->aux = io->base + 0x200;
> +		io->aux_len = 0x200;
> +		io->link = io->base + 0x400;
> +		io->link_len = 0x600;
> +		io->p0 = io->base + 0xa00;
> +		io->p0_len = 0x400;
> +	} else {
> +		io->ahb = io->base + 0x0;
> +		io->ahb_len = 0x200;
> +		io->aux = io->base + 0x200;
> +		io->aux_len = 0x200;
> +		io->link = io->base + 0x400;
> +		io->link_len = 0x600;
> +		io->p0 = io->base + 0x1000;
> +		io->p0_len = 0x400;
> +	}
> 
>  	return 0;
>  }

Re: [Freedreno] [PATCH 4/4] drm/msm/dp: Add support for SC8180x eDP

[Bjorn Andersson]

On Fri 28 May 18:40 CDT 2021, abhinavk@codeaurora.org wrote:

> On 2021-05-10 21:20, Bjorn Andersson wrote:
> > The eDP controller found in SC8180x is at large compatible with the
> > current implementation, but has its register blocks at slightly
> > different offsets.
> > 
> > Add the compatible and the new register layout.
> > 
> I am not able to completely recall the history of why in the DP bindings
> we added DP register base as a big hunk and let catalog handle the submodule
> offsets.
> 
> I guess earlier that made sense because DP sub-block offsets were fixed.
> But if we plan to re-use the DP driver for eDP as well like this series,
> then maybe it might be
> better if this comes from device tree like the earlier version was planning
> to
> 
> https://patchwork.kernel.org/project/dri-devel/patch/0101016ec6ddf446-e87ab1ce-5cbf-40a0-a0bb-cd0151cd577a-000000@us-west-2.amazonses.com/
> 
> 
> +- reg:                  Base address and length of DP hardware's memory
> mapped regions.
> +- cell-index:           Specifies the controller instance.
> +- reg-names:            A list of strings that name the list of regs.
> +			"dp_ahb" - DP controller memory region.
> +			"dp_aux" - DP AUX memory region.
> +			"dp_link" - DP link layer memory region.
> +			"dp_p0" - DP pixel clock domain memory region.
> +			"dp_phy" - DP PHY memory region.
> +			"dp_ln_tx0" - USB3 DP PHY combo TX-0 lane memory region.
> +			"dp_ln_tx1" - USB3 DP PHY combo TX-1 lane memory region.
> 
> Now there is more reason to separate the sub-module offsets like
> ahb/aux/link/p0

I like it, will rewrite the patch accordingly.

Regards,
Bjorn

> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  drivers/gpu/drm/msm/dp/dp_display.c |  1 +
> >  drivers/gpu/drm/msm/dp/dp_parser.c  | 28 ++++++++++++++++++++--------
> >  2 files changed, 21 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index d1319b58e901..0be03bdc882c 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -121,6 +121,7 @@ struct dp_display_private {
> > 
> >  static const struct of_device_id dp_dt_match[] = {
> >  	{.compatible = "qcom,sc7180-dp"},
> > +	{ .compatible = "qcom,sc8180x-edp" },
> >  	{}
> >  };
> > 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> > b/drivers/gpu/drm/msm/dp/dp_parser.c
> > index 51ec85b4803b..47cf18bba4b2 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> > @@ -251,6 +251,7 @@ static int dp_parser_clock(struct dp_parser *parser)
> >  static int dp_parser_parse(struct dp_parser *parser)
> >  {
> >  	struct dss_io_data *io = &parser->io.dp_controller;
> > +	struct device *dev = &parser->pdev->dev;
> >  	int rc = 0;
> > 
> >  	if (!parser) {
> > @@ -276,14 +277,25 @@ static int dp_parser_parse(struct dp_parser
> > *parser)
> >  	 */
> >  	parser->regulator_cfg = &sdm845_dp_reg_cfg;
> > 
> > -	io->ahb = io->base + 0x0;
> > -	io->ahb_len = 0x200;
> > -	io->aux = io->base + 0x200;
> > -	io->aux_len = 0x200;
> > -	io->link = io->base + 0x400;
> > -	io->link_len = 0x600;
> > -	io->p0 = io->base + 0x1000;
> > -	io->p0_len = 0x400;
> > +	if (of_device_is_compatible(dev->of_node, "qcom,sc8180x-edp")) {
> > +		io->ahb = io->base + 0x0;
> > +		io->ahb_len = 0x200;
> > +		io->aux = io->base + 0x200;
> > +		io->aux_len = 0x200;
> > +		io->link = io->base + 0x400;
> > +		io->link_len = 0x600;
> > +		io->p0 = io->base + 0xa00;
> > +		io->p0_len = 0x400;
> > +	} else {
> > +		io->ahb = io->base + 0x0;
> > +		io->ahb_len = 0x200;
> > +		io->aux = io->base + 0x200;
> > +		io->aux_len = 0x200;
> > +		io->link = io->base + 0x400;
> > +		io->link_len = 0x600;
> > +		io->p0 = io->base + 0x1000;
> > +		io->p0_len = 0x400;
> > +	}
> > 
> >  	return 0;
> >  }

Re: [Freedreno] [PATCH 4/4] drm/msm/dp: Add support for SC8180x eDP

[Stephen Boyd]

Quoting abhinavk@codeaurora.org (2021-05-28 16:40:32)
> On 2021-05-10 21:20, Bjorn Andersson wrote:
> > The eDP controller found in SC8180x is at large compatible with the
> > current implementation, but has its register blocks at slightly
> > different offsets.
> >
> > Add the compatible and the new register layout.
> >
> I am not able to completely recall the history of why in the DP bindings
> we added DP register base as a big hunk and let catalog handle the
> submodule
> offsets.

I complained that there were many I/O regions for the DP block that
didn't seem to be changing between SoCs. Nobody objected to removing it
back then, but if the plan was to move things around later on then it
makes sense to split it out like it was done initially.

>
> I guess earlier that made sense because DP sub-block offsets were fixed.
> But if we plan to re-use the DP driver for eDP as well like this series,
> then maybe it might be
> better if this comes from device tree like the earlier version was
> planning to
>
> https://patchwork.kernel.org/project/dri-devel/patch/0101016ec6ddf446-e87ab1ce-5cbf-40a0-a0bb-cd0151cd577a-000000@us-west-2.amazonses.com/
>

Agreed.

Re: [Freedreno] [PATCH 1/4] drm/msm/dp: Simplify the mvid/nvid calculation

[Bjorn Andersson]

On Fri 28 May 18:11 CDT 2021, abhinavk@codeaurora.org wrote:

> Hi Bjorn
> 
> On 2021-05-10 21:20, Bjorn Andersson wrote:
> > In the search for causes to timing issues seen during implementation of
> > eDP support for SC8180x a fair amount of time was spent concluding why
> > the calculated mvid/nvid values where wrong.
> > 
> > The overall conclusion is that the ratio of MVID/NVID describes, and
> > should match, the ratio between the pixel and link clock.
> > 
> > Downstream this calculation reads the M and N values off the pixel clock
> > straight from DISP_CC and are then adjusted based on knowledge of how
> > the link and vco_div (parent of the pixel clock) are derrived from the
> > common VCO.
> > 
> > While upstreaming, and then extracting the PHY driver, the resulting
> > function performs the following steps:
> > 
> > 1) Adjust the passed link rate based on the VCO divider used in the PHY
> >    driver, and multiply this by 10 based on the link rate divider.
> > 2) Pick reasonable choices of M and N, by calculating the ratio between
> >    this new clock and the pixel clock.
> > 3) Subtract M from N and flip the bits, to match the encoding of the N
> >    register in DISP_CC.
> > 4) Flip the bits of N and add M, to get the value of N back.
> > 5) Multiply M with 5, per the documentation.
> > 6) Scale the values such that N is close to 0x8000 (or larger)
> > 7) Multply M with 2 or 3 depending on the link rate of HBR2 or HBR3.
> > 
> > Presumably step 3) was added to provide step 4) with expected input, so
> > the two cancel each other out. The factor of 10 from step 1) goes into
> > the denominator and is partially cancelled by the 5 in the numerator in
> > step 5), resulting in step 7) simply cancelling out step 1).
> > 
> 
> Both the multiplication of M with 5 and N with 2 or 3 is coming because of
> the
> ratio between the vco clk and the link clk.
> So we could have 2.7, 5.4 or 8.1 Gbps link clks and the factor of 2 or 3
> gets added because hbr2 is 2 * hbr and hbr3 is 3 * hbr.
> 

Thanks for explaining the origin of these numbers, I had quite a
difficult time figuring out where the "magic" came from.

> Your summary is pretty much right otherwise. Let me add some more points
> here:
> 
> 1) Originally we removed reading the M_VID and N_VID from the DISPCC regs
> because
> of previous upstream comments that we can potentially just recalculate
> whatever the clk driver is programming
> by using rational_best_approximation
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/clk/qcom/clk-rcg2.c#L1160
> 
> Not having to read from DISPCC register is also useful because we dont have
> to maintain the register offset
> of the M_VID and N_VID which keeps changing across chipsets.
> 

Right, so downstream we do all the math and then we scale the
denominator by 2x or 3x to compensate for the fact that we didn't
account for the division as the clock left the PLL.

As this was reworked upstream for some reason this compensation was
retained, so the denominator would always be 2x or 3x to large for HBR2
and HBR3. So the way this was solved was to divide by 2x or 3x before
calculating the ratio.

> However we discussed this again after viewing this patch. So the clk driver
> always operates on the vco clk
> and calculates the pixel clk from it and sets the M_VID and N_VID based on
> that.
> In terms of accuracy, the best way is still to re-use the M_VID and N_VID
> which the clk driver sets because
> the pixel clock was generated based on that and that is the actual pixel
> clock we are going to get.
> 
> So even before this change we lost some accuracy because the pixel clock we
> are giving here to recalculate
> the M_VID and N_VID is a theoretical value. Although for most values of
> pixel clk, theoretical and actual
> should match. There could be corner cases of pixel clock where its a bit
> different. Hence ideally, re-using the M_VID
> and N_VID which the clk driver set would have been the best but not having
> to hard-code M_VID and N_VID offsets
> was a good enough reason to not go back to that again.
> 
> Now, coming to this change. Here its trying to again re-calculate the M_VID
> and N_VID by using the same
> API which the clk driver uses but uses link clk and pixel clk as the
> parameters Vs the clk driver uses
> vco clk and actual pixel clock to calculate this.
> 
> So even though this cleanup eliminates the adjustments we need to make to
> account for the VCO clk to link clk ratio,
> it also could bring additional difference between what was actually set by
> the clk driver and what we are calculating
> here because clk driver used vco clk as the input vs here we use link clk
> after this change.
> There might be some pixel clock rates of some resolutions where this
> difference could be risky.
> 
> Hence the overall conclusion here was to keep using vco clk as the input to
> rational_best_approximation
> and not make more changes to this.
> 

So what you're saying is that the reason for this obfuscation is to
replicate any rounding errors happening in the path of the link clock
generation?

If that's the case then this needs a giant comment describing exactly
what's happening and why this function needs to be impenetrable.


That said, from my attempts to write this patch (and add widebus
support) I saw a huge flexibility in getting this right, so can you
please elaborate on the value of the precision of the ratio.

Regards,
Bjorn

Re: [Freedreno] [PATCH 1/4] drm/msm/dp: Simplify the mvid/nvid calculation

[abhinavk]

Hi Bjorn

On 2021-06-08 13:13, Bjorn Andersson wrote:
> On Fri 28 May 18:11 CDT 2021, abhinavk@codeaurora.org wrote:
> 
>> Hi Bjorn
>> 
>> On 2021-05-10 21:20, Bjorn Andersson wrote:
>> > In the search for causes to timing issues seen during implementation of
>> > eDP support for SC8180x a fair amount of time was spent concluding why
>> > the calculated mvid/nvid values where wrong.
>> >
>> > The overall conclusion is that the ratio of MVID/NVID describes, and
>> > should match, the ratio between the pixel and link clock.
>> >
>> > Downstream this calculation reads the M and N values off the pixel clock
>> > straight from DISP_CC and are then adjusted based on knowledge of how
>> > the link and vco_div (parent of the pixel clock) are derrived from the
>> > common VCO.
>> >
>> > While upstreaming, and then extracting the PHY driver, the resulting
>> > function performs the following steps:
>> >
>> > 1) Adjust the passed link rate based on the VCO divider used in the PHY
>> >    driver, and multiply this by 10 based on the link rate divider.
>> > 2) Pick reasonable choices of M and N, by calculating the ratio between
>> >    this new clock and the pixel clock.
>> > 3) Subtract M from N and flip the bits, to match the encoding of the N
>> >    register in DISP_CC.
>> > 4) Flip the bits of N and add M, to get the value of N back.
>> > 5) Multiply M with 5, per the documentation.
>> > 6) Scale the values such that N is close to 0x8000 (or larger)
>> > 7) Multply M with 2 or 3 depending on the link rate of HBR2 or HBR3.
>> >
>> > Presumably step 3) was added to provide step 4) with expected input, so
>> > the two cancel each other out. The factor of 10 from step 1) goes into
>> > the denominator and is partially cancelled by the 5 in the numerator in
>> > step 5), resulting in step 7) simply cancelling out step 1).
>> >
>> 
>> Both the multiplication of M with 5 and N with 2 or 3 is coming 
>> because of
>> the
>> ratio between the vco clk and the link clk.
>> So we could have 2.7, 5.4 or 8.1 Gbps link clks and the factor of 2 or 
>> 3
>> gets added because hbr2 is 2 * hbr and hbr3 is 3 * hbr.
>> 
> 
> Thanks for explaining the origin of these numbers, I had quite a
> difficult time figuring out where the "magic" came from.
> 
>> Your summary is pretty much right otherwise. Let me add some more 
>> points
>> here:
>> 
>> 1) Originally we removed reading the M_VID and N_VID from the DISPCC 
>> regs
>> because
>> of previous upstream comments that we can potentially just recalculate
>> whatever the clk driver is programming
>> by using rational_best_approximation
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/clk/qcom/clk-rcg2.c#L1160
>> 
>> Not having to read from DISPCC register is also useful because we dont 
>> have
>> to maintain the register offset
>> of the M_VID and N_VID which keeps changing across chipsets.
>> 
> 
> Right, so downstream we do all the math and then we scale the
> denominator by 2x or 3x to compensate for the fact that we didn't
> account for the division as the clock left the PLL.
> 
> As this was reworked upstream for some reason this compensation was
> retained, so the denominator would always be 2x or 3x to large for HBR2
> and HBR3. So the way this was solved was to divide by 2x or 3x before
> calculating the ratio.
> 
>> However we discussed this again after viewing this patch. So the clk 
>> driver
>> always operates on the vco clk
>> and calculates the pixel clk from it and sets the M_VID and N_VID 
>> based on
>> that.
>> In terms of accuracy, the best way is still to re-use the M_VID and 
>> N_VID
>> which the clk driver sets because
>> the pixel clock was generated based on that and that is the actual 
>> pixel
>> clock we are going to get.
>> 
>> So even before this change we lost some accuracy because the pixel 
>> clock we
>> are giving here to recalculate
>> the M_VID and N_VID is a theoretical value. Although for most values 
>> of
>> pixel clk, theoretical and actual
>> should match. There could be corner cases of pixel clock where its a 
>> bit
>> different. Hence ideally, re-using the M_VID
>> and N_VID which the clk driver set would have been the best but not 
>> having
>> to hard-code M_VID and N_VID offsets
>> was a good enough reason to not go back to that again.
>> 
>> Now, coming to this change. Here its trying to again re-calculate the 
>> M_VID
>> and N_VID by using the same
>> API which the clk driver uses but uses link clk and pixel clk as the
>> parameters Vs the clk driver uses
>> vco clk and actual pixel clock to calculate this.
>> 
>> So even though this cleanup eliminates the adjustments we need to make 
>> to
>> account for the VCO clk to link clk ratio,
>> it also could bring additional difference between what was actually 
>> set by
>> the clk driver and what we are calculating
>> here because clk driver used vco clk as the input vs here we use link 
>> clk
>> after this change.
>> There might be some pixel clock rates of some resolutions where this
>> difference could be risky.
>> 
>> Hence the overall conclusion here was to keep using vco clk as the 
>> input to
>> rational_best_approximation
>> and not make more changes to this.
>> 
> 
> So what you're saying is that the reason for this obfuscation is to
> replicate any rounding errors happening in the path of the link clock
> generation?
> 
> If that's the case then this needs a giant comment describing exactly
> what's happening and why this function needs to be impenetrable.
> 
> 
> That said, from my attempts to write this patch (and add widebus
> support) I saw a huge flexibility in getting this right, so can you
> please elaborate on the value of the precision of the ratio.
> 

The overall goal here is just to replicate whats happening in the clock 
driver and clock hardware to calculate the pixel clock.
That is, use the same inputs and function as the ones used in clk driver 
to calculated to set the DISPCC_MVID and DISPCC_NVID regs.

I think i have already explained why we need the *2 and *3 in the math. 
If you need, sure I can document this as well in code
comments.

By obfuscation are you referring to this snippet?
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/dp/dp_catalog.c#L446

This is just recalculating the dispcc rate from the pixel clock rate so 
that we feed the same input
to the rational_best_approximation as what the clock driver would do and 
hence making our input
and output same for the rational_best_approximation as the clock driver 
to maintain consistency and I will explain why.

Let me explain a little bit more on the mnd precision i am referring to.

The clock driver will also use the same API to calculate the mnds and 
generate a pixel clock.
It might not match the requested theoretical pixel clock as the mnds 
calculated with this API might have some precision errors.

rational_best_approximation(dispcc_input_rate, stream_rate_khz,
			(unsigned long)(1 << 16) - 1,
			(unsigned long)(1 << 16) - 1, &den, &num);

By using the same input (dispcc rate) and output(pixel clk) rates, we 
are getting the same mnds and hence same mvid and nvid.

If you use the link clk rate here instead, it might generate some other 
mvid/nvid and the pixel clock for that might be which is different
from what is the actual pixel clock which the PLL generated ( due to the 
mnd calculated using the API ).

Hence to preserve the same loss of accuracy which the clock driver would 
have, we would prefer to use the same input here too.

Let me know if its still not clear.

> Regards,
> Bjorn