diff mbox series

[RFC,10/11] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

Message ID 20210322030128.2283-11-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State New
Delegated to: Kieran Bingham
Headers show
Series drm/bridge: ti-sn65dsi86: Support DisplayPort mode | expand

Commit Message

Laurent Pinchart March 22, 2021, 3:01 a.m. UTC
Despite the SN65DSI86 being an eDP bridge, on some systems its output is
routed to a DisplayPort connector. Enable DisplayPort mode when the next
component in the display pipeline is not a panel, and disable eDP
features in that case.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 32 ++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 8 deletions(-)

Comments

Doug Anderson March 24, 2021, 10:47 p.m. UTC | #1
Hi,

On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
>
> Despite the SN65DSI86 being an eDP bridge, on some systems its output is
> routed to a DisplayPort connector. Enable DisplayPort mode when the next
> component in the display pipeline is not a panel, and disable eDP
> features in that case.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 32 ++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index e2527d597ccb..f792227142a7 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -55,6 +55,7 @@
>  #define SN_LN_ASSIGN_REG                       0x59
>  #define  LN_ASSIGN_WIDTH                       2
>  #define SN_ENH_FRAME_REG                       0x5A
> +#define  ASSR_CONTROL                          BIT(0)
>  #define  VSTREAM_ENABLE                                BIT(3)
>  #define  LN_POLRS_OFFSET                       4
>  #define  LN_POLRS_MASK                         0xf0
> @@ -86,6 +87,8 @@
>  #define SN_DATARATE_CONFIG_REG                 0x94
>  #define  DP_DATARATE_MASK                      GENMASK(7, 5)
>  #define  DP_DATARATE(x)                                ((x) << 5)
> +#define SN_TRAINING_SETTING_REG                        0x95
> +#define  SCRAMBLE_DISABLE                      BIT(4)
>  #define SN_ML_TX_MODE_REG                      0x96
>  #define  ML_TX_MAIN_LINK_OFF                   0
>  #define  ML_TX_NORMAL_MODE                     BIT(0)
> @@ -723,6 +726,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx,
>         regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
>                            DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
>
> +       /* For DisplayPort, use the standard DP scrambler seed. */
> +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> +               regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
> +                                  ASSR_CONTROL, 0);

I don't actually know anything about DP scrambler seeds. However:

1. From reading the docs, this field seems to be documented to be
"read only" unless:

1a) The "TEST2" pin is pulled high when you power on the bridge.
1b) You set "ASSR_OVERRIDE" (page select to page 7, write to register
0x16, page select back to page 0).

I don't know if TEST2 is being pulled high in your hardware, but at
least I can see that 1b) isn't done. So I'm guessing that this line is
a no-op? If I had to guess from all the hoops they're making you jump
through there's some sort of errata around standard scrambling on this
bridge chip. Are you sure it works OK?


2. The docs I see claim that this field is 2 bits big. It seems like
it would be nice to honor. Yeah, it's silly because 0x11 and 0x10 are
"reserved" so it's really more like a 1-bit field, but still seems
like it would be better to set both bits, or at least add a comment
explaining why you're not matching the datasheet.


3. Your patch doesn't seem to touch the bit of code in
ti_sn_bridge_enable() that says this:

/**
 * The SN65DSI86 only supports ASSR Display Authentication method and
 * this method is enabled by default. An eDP panel must support this
 * authentication method. We need to enable this method in the eDP panel
 * at DisplayPort address 0x0010A prior to link training.
 */
drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
   DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);

Won't that be a problem?


> +
>         /* enable DP PLL */
>         regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
>
> @@ -734,6 +742,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx,
>                 goto exit;
>         }
>
> +       /* For DisplayPort, disable scrambling mode. */
> +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> +               regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
> +                                  SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);

I'm assuming that this is the important part of your patch? Would be
sorta nice to include the "why" in your comment. Why do you want to
disable scrambling mode for DP but not for eDP? Maybe you care about
compatibility but not EMI if you're hooking up to random DP things?

-Doug
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index e2527d597ccb..f792227142a7 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -55,6 +55,7 @@ 
 #define SN_LN_ASSIGN_REG			0x59
 #define  LN_ASSIGN_WIDTH			2
 #define SN_ENH_FRAME_REG			0x5A
+#define  ASSR_CONTROL				BIT(0)
 #define  VSTREAM_ENABLE				BIT(3)
 #define  LN_POLRS_OFFSET			4
 #define  LN_POLRS_MASK				0xf0
@@ -86,6 +87,8 @@ 
 #define SN_DATARATE_CONFIG_REG			0x94
 #define  DP_DATARATE_MASK			GENMASK(7, 5)
 #define  DP_DATARATE(x)				((x) << 5)
+#define SN_TRAINING_SETTING_REG			0x95
+#define  SCRAMBLE_DISABLE			BIT(4)
 #define SN_ML_TX_MODE_REG			0x96
 #define  ML_TX_MAIN_LINK_OFF			0
 #define  ML_TX_NORMAL_MODE			BIT(0)
@@ -723,6 +726,11 @@  static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx,
 	regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
 			   DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
 
+	/* For DisplayPort, use the standard DP scrambler seed. */
+	if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+		regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
+				   ASSR_CONTROL, 0);
+
 	/* enable DP PLL */
 	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
 
@@ -734,6 +742,11 @@  static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx,
 		goto exit;
 	}
 
+	/* For DisplayPort, disable scrambling mode. */
+	if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+		regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
+				   SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
+
 	/*
 	 * We'll try to link train several times.  As part of link training
 	 * the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER.  If
@@ -1288,18 +1301,20 @@  static int ti_sn_bridge_probe(struct i2c_client *client,
 	pdata->dev = &client->dev;
 
 	ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, 0,
-					  &pdata->panel, NULL);
+					  &pdata->panel, &pdata->next_bridge);
 	if (ret) {
 		DRM_ERROR("could not find any panel node\n");
 		return ret;
 	}
 
-	pdata->next_bridge = devm_drm_panel_bridge_add(pdata->dev,
-						       pdata->panel);
-	if (IS_ERR(pdata->next_bridge)) {
-		DRM_ERROR("failed to create panel bridge\n");
-		ret = PTR_ERR(pdata->next_bridge);
-		return ret;
+	if (!pdata->next_bridge) {
+		pdata->next_bridge = devm_drm_panel_bridge_add(pdata->dev,
+							       pdata->panel);
+		if (IS_ERR(pdata->next_bridge)) {
+			DRM_ERROR("failed to create panel bridge\n");
+			ret = PTR_ERR(pdata->next_bridge);
+			return ret;
+		}
 	}
 
 	dev_set_drvdata(&client->dev, pdata);
@@ -1351,7 +1366,8 @@  static int ti_sn_bridge_probe(struct i2c_client *client,
 	pdata->bridge.funcs = &ti_sn_bridge_funcs;
 	pdata->bridge.of_node = client->dev.of_node;
 	pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
-	pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
+	pdata->bridge.type = pdata->panel ? DRM_MODE_CONNECTOR_eDP
+			   : DRM_MODE_CONNECTOR_DisplayPort;
 
 	drm_bridge_add(&pdata->bridge);