2022-11-19 17:33:52

by Aradhya Bhatia

[permalink] [raw]
Subject: [PATCH v6 0/5] Add DSS support for AM625 SoC

This patch series adds a new compatible for the Display SubSyetem
controller on TI's AM625 SoC. It further adds the required support for
the same in the tidss driver.

The AM625-DSS is a newer version of the DSS from the AM65X version with
the major change being the addition of another OLDI TX. With the help of
2 OLDI TXes, the AM625 DSS can support dual-linked OLDI displays with a
resolution of upto 2K or WUXGA (1920x1200@60fps) at half the OLDI clock
frequency or even cloned video outputs on each of the TXes.

TODO:
- The pixel clock for the OLDI VP passes through a clock divider, which
was being explicitly set in previous versions, but that was not the
right way. That patch was dropped and a newer implementation is in
works.

Note:
- The roots of this patch set can be found in the following series.
https://patchwork.kernel.org/project/dri-devel/list/?series=660970&state=%2A&archive=both

The changes in the above-mentioned series forced some re-works in
this series, and since all the patches were better understood as a
single set, both the series were combined.

- The previous patch series couldn't take into account OLDI bridges
that worked with Clone / Dual Link Mode. That has been rectified in
this patch set. This became possible because the OLDI mode discovery
was separated from the panel/bridge discovery loop during modeset
initialization.

Changelog:
V6:
- Rebase for current merge window.
- Add 'allOf:' condition in the DT binding.
- Address Tomi Valkeinen's comments
1. Combine DT binding patches for new compatible and 3rd DSS port.
2. Further separate DSS VPs and output ports.
3. Separate OLDI mode discovery logic from the panel/bridge discovery
(which allowed support for OLDI bridges as well.)
4. Organize OLDI IO control regsiter macros platform wise.

V5:
- Rebase for current merge window
- Add max DT ports in DSS features
- Combine the OLDI support series

(Changes from OLDI support series v1)
- Address Tomi Valkeinen's comments
1. Update the OLDI link detection approach
2. Add port #3 for 2nd OLDI TX
3. Configure 2 panel-bridges for cloned panels
4. Drop the OLDI clock set patch
5. Drop rgb565-to-888 patch

V4:
- Rebase for current merge window
- Add acked and reviewed by tags

V3:
- Change yaml enum in alphabetical order
- Correct a typo

V2:
- Remove redundant regsiter array

Aradhya Bhatia (5):
dt-bindings: display: ti,am65x-dss: Add support for am625 dss
drm/tidss: Add support for AM625 DSS
drm/tidss: Add support to configure OLDI mode for am625-dss.
drm/tidss: Add IO CTRL and Power support for OLDI TX in am625
drm/tidss: Enable Dual and Duplicate Modes for OLDI

.../bindings/display/ti/ti,am65x-dss.yaml | 23 ++-
drivers/gpu/drm/tidss/tidss_dispc.c | 171 ++++++++++++++--
drivers/gpu/drm/tidss/tidss_dispc.h | 24 ++-
drivers/gpu/drm/tidss/tidss_dispc_regs.h | 37 +++-
drivers/gpu/drm/tidss/tidss_drv.c | 1 +
drivers/gpu/drm/tidss/tidss_drv.h | 8 +-
drivers/gpu/drm/tidss/tidss_encoder.c | 4 +-
drivers/gpu/drm/tidss/tidss_encoder.h | 3 +-
drivers/gpu/drm/tidss/tidss_irq.h | 2 +-
drivers/gpu/drm/tidss/tidss_kms.c | 188 ++++++++++++++++--
10 files changed, 400 insertions(+), 61 deletions(-)

--
2.38.1



2022-11-19 17:46:47

by Aradhya Bhatia

[permalink] [raw]
Subject: [PATCH v6 3/5] drm/tidss: Add support to configure OLDI mode for am625-dss.

The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal.
These can be configured to support the following modes:

1. OLDI_SINGLE_LINK_SINGLE_MODE
Single Output over OLDI 0.
+------+ +---------+ +-------+
| | | | | |
| CRTC +------->+ ENCODER +----->| PANEL |
| | | | | |
+------+ +---------+ +-------+

2. OLDI_SINGLE_LINK_CLONE_MODE
Duplicate Output over OLDI 0 and 1.
+------+ +---------+ +-------+
| | | | | |
| CRTC +---+--->| ENCODER +----->| PANEL |
| | | | | | |
+------+ | +---------+ +-------+
|
| +---------+ +-------+
| | | | |
+--->| ENCODER +----->| PANEL |
| | | |
+---------+ +-------+

3. OLDI_DUAL_LINK_MODE
Combined Output over OLDI 0 and 1.
+------+ +---------+ +-------+
| | | +----->| |
| CRTC +------->+ ENCODER | | PANEL |
| | | +----->| |
+------+ +---------+ +-------+

Following the above pathways for different modes, 2 encoder/panel-bridge
pipes get created for clone mode, and 1 pipe in cases of single link and
dual link mode.

Add support for confguring the OLDI modes using OF and LVDS DRM helper
functions.

Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/tidss/tidss_dispc.c | 12 ++
drivers/gpu/drm/tidss/tidss_dispc.h | 9 ++
drivers/gpu/drm/tidss/tidss_drv.h | 3 +
drivers/gpu/drm/tidss/tidss_encoder.c | 4 +-
drivers/gpu/drm/tidss/tidss_encoder.h | 3 +-
drivers/gpu/drm/tidss/tidss_kms.c | 188 +++++++++++++++++++++++---
6 files changed, 198 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index dbc6a5b617ca..472226a83251 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -365,6 +365,8 @@ struct dispc_device {

struct dss_vp_data vp_data[TIDSS_MAX_VPS];

+ enum dispc_oldi_modes oldi_mode;
+
u32 *fourccs;
u32 num_fourccs;

@@ -1967,6 +1969,16 @@ const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len)
return dispc->fourccs;
}

+int dispc_set_oldi_mode(struct dispc_device *dispc,
+ enum dispc_oldi_modes oldi_mode)
+{
+ WARN_ON(!dispc);
+
+ dispc->oldi_mode = oldi_mode;
+
+ return 0;
+}
+
static s32 pixinc(int pixels, u8 ps)
{
if (pixels == 1)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index 51db500590ee..e76a7599b544 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -64,6 +64,14 @@ enum dispc_dss_subrevision {
DISPC_AM625,
};

+enum dispc_oldi_modes {
+ OLDI_MODE_OFF, /* OLDI turned off / tied off in IP. */
+ OLDI_SINGLE_LINK_SINGLE_MODE, /* Single Output over OLDI 0. */
+ OLDI_SINGLE_LINK_CLONE_MODE, /* Duplicate Output over OLDI 0 and 1. */
+ OLDI_DUAL_LINK_MODE, /* Combined Output over OLDI 0 and 1. */
+ OLDI_MODE_UNSUPPORTED, /* Unsupported OLDI Mode */
+};
+
struct dispc_features {
int min_pclk_khz;
int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
@@ -133,6 +141,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
u32 hw_videoport);
int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable);
const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len);
+int dispc_set_oldi_mode(struct dispc_device *dispc, enum dispc_oldi_modes oldi_mode);

int dispc_init(struct tidss_device *tidss);
void dispc_remove(struct tidss_device *tidss);
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
index 0ce7ee5ccd5b..58892f065c16 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -13,6 +13,9 @@
#define TIDSS_MAX_PLANES 4
#define TIDSS_MAX_OUTPUT_PORTS 4

+/* For AM625-DSS with 2 OLDI TXes */
+#define TIDSS_MAX_BRIDGES_PER_PIPE 2
+
typedef u32 dispc_irq_t;

struct tidss_device {
diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
index e278a9c89476..141383ec4045 100644
--- a/drivers/gpu/drm/tidss/tidss_encoder.c
+++ b/drivers/gpu/drm/tidss/tidss_encoder.c
@@ -70,7 +70,8 @@ static const struct drm_encoder_funcs encoder_funcs = {
};

struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
- u32 encoder_type, u32 possible_crtcs)
+ u32 encoder_type, u32 possible_crtcs,
+ u32 possible_clones)
{
struct drm_encoder *enc;
int ret;
@@ -80,6 +81,7 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
return ERR_PTR(-ENOMEM);

enc->possible_crtcs = possible_crtcs;
+ enc->possible_clones = possible_clones;

ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs,
encoder_type, NULL);
diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h b/drivers/gpu/drm/tidss/tidss_encoder.h
index ace877c0e0fd..01c62ba3ef16 100644
--- a/drivers/gpu/drm/tidss/tidss_encoder.h
+++ b/drivers/gpu/drm/tidss/tidss_encoder.h
@@ -12,6 +12,7 @@
struct tidss_device;

struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
- u32 encoder_type, u32 possible_crtcs);
+ u32 encoder_type, u32 possible_crtcs,
+ u32 possible_clones);

#endif
diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
index fc9c4eefd31d..8ae321f02197 100644
--- a/drivers/gpu/drm/tidss/tidss_kms.c
+++ b/drivers/gpu/drm/tidss/tidss_kms.c
@@ -106,30 +106,115 @@ static const struct drm_mode_config_funcs mode_config_funcs = {
.atomic_commit = drm_atomic_helper_commit,
};

+static enum dispc_oldi_modes tidss_get_oldi_mode(struct device_node *oldi0_port,
+ struct device_node *oldi1_port)
+{
+ int pixel_order;
+
+ if (!(oldi0_port || oldi1_port)) {
+ /* Keep OLDI TXes off if neither OLDI port is present. */
+ return OLDI_MODE_OFF;
+ } else if (oldi0_port && !oldi1_port) {
+ /*
+ * OLDI0 port found, but not OLDI1 port. Setting single
+ * link, single mode output.
+ */
+ return OLDI_SINGLE_LINK_SINGLE_MODE;
+ } else if (!oldi0_port && oldi1_port) {
+ /*
+ * The 2nd OLDI TX cannot be operated alone. This use case is
+ * not supported in the HW. Since the pins for OLDIs 0 and 1 are
+ * separate, one could theoretically set a clone mode over OLDIs
+ * 0 and 1 and just simply not use the OLDI 0. This is a hacky
+ * way to enable only OLDI TX 1 and hence is not officially
+ * supported.
+ */
+ return OLDI_MODE_UNSUPPORTED;
+ }
+
+ /*
+ * OLDI Ports found for both the OLDI TXes. The DSS is to be configured
+ * in either Dual Link or Clone Mode.
+ */
+ pixel_order = drm_of_lvds_get_dual_link_pixel_order(oldi0_port,
+ oldi1_port);
+ switch (pixel_order) {
+ case -EINVAL:
+ /*
+ * The dual link properties were not found in at least one of
+ * the sink nodes. Since 2 OLDI ports are present in the DT, it
+ * can be safely assumed that the required configuration is
+ * Clone Mode.
+ */
+ return OLDI_SINGLE_LINK_CLONE_MODE;
+
+ case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
+ /*
+ * Note that the OLDI TX 0 transmits the odd set of pixels while
+ * the OLDI TX 1 transmits the even set. This is a fixed
+ * configuration in the IP and an cannot be change vis SW. These
+ * properties have been used to merely identify if a Dual Link
+ * configuration is required. Swapping this property in the panel
+ * port DT nodes will not make any difference.
+ */
+ pr_warn("EVEN-ODD config for dual-link sinks is not supported in HW. Switching to ODD-EVEN.\n");
+ fallthrough;
+
+ case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
+ return OLDI_DUAL_LINK_MODE;
+
+ default:
+ return OLDI_MODE_OFF;
+ }
+}
+
static int tidss_dispc_modeset_init(struct tidss_device *tidss)
{
struct device *dev = tidss->dev;
unsigned int fourccs_len;
const u32 *fourccs = dispc_plane_formats(tidss->dispc, &fourccs_len);
- unsigned int i;
+ unsigned int i, j;

struct pipe {
u32 hw_videoport;
- struct drm_bridge *bridge;
+ struct drm_bridge *bridge[TIDSS_MAX_BRIDGES_PER_PIPE];
u32 enc_type;
+ u32 num_bridges;
};

const struct dispc_features *feat = tidss->feat;
- u32 max_vps = feat->num_vps;
+ u32 output_ports = feat->num_output_ports;
u32 max_planes = feat->num_planes;

- struct pipe pipes[TIDSS_MAX_VPS];
+ struct pipe pipes[TIDSS_MAX_VPS] = {0};
+
u32 num_pipes = 0;
u32 crtc_mask;
+ u32 portnum_oldi0 = 0, portnum_oldi1 = 2;
+ enum dispc_oldi_modes oldi_mode = OLDI_MODE_OFF;
+ u32 num_oldi = 0;
+ u32 oldi_pipe_index = 0;
+ u32 num_encoders = 0;
+
+ if (feat->oldi_supported) {
+ struct device_node *oldi0_port, *oldi1_port;
+
+ oldi0_port = of_graph_get_port_by_id(dev->of_node,
+ portnum_oldi0);
+ oldi1_port = of_graph_get_port_by_id(dev->of_node,
+ portnum_oldi1);
+
+ oldi_mode = tidss_get_oldi_mode(oldi0_port, oldi1_port);
+
+ of_node_put(oldi0_port);
+ of_node_put(oldi1_port);
+
+ dispc_set_oldi_mode(tidss->dispc, oldi_mode);
+ }

/* first find all the connected panels & bridges */

- for (i = 0; i < max_vps; i++) {
+ for (i = 0; i < output_ports; i++) {
struct drm_panel *panel;
struct drm_bridge *bridge;
u32 enc_type = DRM_MODE_ENCODER_NONE;
@@ -145,16 +230,24 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
return ret;
}

+ if (feat->output_port_bus_type[i] == DISPC_VP_OLDI &&
+ oldi_mode == OLDI_MODE_UNSUPPORTED) {
+ dev_err(dev,
+ "Single Mode over OLDI 1 is not supported in HW. Keeping OLDI off.\n");
+ continue;
+ }
+
if (panel) {
u32 conn_type;

dev_dbg(dev, "Setting up panel for port %d\n", i);

- switch (feat->vp_bus_type[i]) {
+ switch (feat->output_port_bus_type[i]) {
case DISPC_VP_OLDI:
enc_type = DRM_MODE_ENCODER_LVDS;
conn_type = DRM_MODE_CONNECTOR_LVDS;
break;
+
case DISPC_VP_DPI:
enc_type = DRM_MODE_ENCODER_DPI;
conn_type = DRM_MODE_CONNECTOR_DPI;
@@ -172,6 +265,17 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
return -EINVAL;
}

+ /*
+ * If the 2nd OLDI port is discovered connected to a panel
+ * which is not to be connected in the Clone Mode then a
+ * bridge is not required because the detected port is the
+ * 2nd port for the previously connected panel.
+ */
+ if (feat->output_port_bus_type[i] == DISPC_VP_OLDI &&
+ oldi_mode != OLDI_SINGLE_LINK_CLONE_MODE &&
+ num_oldi)
+ break;
+
bridge = devm_drm_panel_bridge_add(dev, panel);
if (IS_ERR(bridge)) {
dev_err(dev,
@@ -181,10 +285,47 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
}
}

- pipes[num_pipes].hw_videoport = i;
- pipes[num_pipes].bridge = bridge;
- pipes[num_pipes].enc_type = enc_type;
- num_pipes++;
+ if (feat->output_port_bus_type[i] == DISPC_VP_OLDI) {
+ if (++num_oldi == 1) {
+ /* Setting up pipe parameters when 1st OLDI port is detected */
+
+ pipes[num_pipes].hw_videoport = i;
+ pipes[num_pipes].enc_type = enc_type;
+
+ /*
+ * Saving the pipe index in case its required for
+ * 2nd OLDI Port.
+ */
+ oldi_pipe_index = num_pipes;
+
+ /*
+ * No additional pipe is required for the 2nd OLDI
+ * port, because the 2nd Encoder -> Bridge connection
+ * is the part of the first OLDI Port pipe.
+ *
+ * num_pipes will only be incremented when the first
+ * OLDI port is discovered.
+ */
+ num_pipes++;
+ }
+
+ /*
+ * Bridge is required to be added only if the detected
+ * port is the first OLDI port or a subsequent port in
+ * Clone Mode.
+ */
+ if (oldi_mode == OLDI_SINGLE_LINK_CLONE_MODE ||
+ num_oldi == 1) {
+ pipes[oldi_pipe_index].bridge[num_oldi - 1] = bridge;
+ pipes[oldi_pipe_index].num_bridges++;
+ }
+ } else {
+ pipes[num_pipes].hw_videoport = i;
+ pipes[num_pipes].bridge[0] = bridge;
+ pipes[num_pipes].num_bridges++;
+ pipes[num_pipes].enc_type = enc_type;
+ num_pipes++;
+ }
}

/* all planes can be on any crtc */
@@ -196,6 +337,7 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
struct tidss_plane *tplane;
struct tidss_crtc *tcrtc;
struct drm_encoder *enc;
+ u32 possible_clones = 0;
u32 hw_plane_id = feat->vid_order[tidss->num_planes];
int ret;

@@ -218,16 +360,24 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)

tidss->crtcs[tidss->num_crtcs++] = &tcrtc->crtc;

- enc = tidss_encoder_create(tidss, pipes[i].enc_type,
- 1 << tcrtc->crtc.index);
- if (IS_ERR(enc)) {
- dev_err(tidss->dev, "encoder create failed\n");
- return PTR_ERR(enc);
- }
+ for (j = 0; j < pipes[i].num_bridges; j++) {
+ if (pipes[i].num_bridges > 1)
+ possible_clones = (((1 << pipes[i].num_bridges) - 1)
+ << num_encoders);
+
+ enc = tidss_encoder_create(tidss, pipes[i].enc_type,
+ 1 << tcrtc->crtc.index,
+ possible_clones);
+ if (IS_ERR(enc)) {
+ dev_err(tidss->dev, "encoder create failed\n");
+ return PTR_ERR(enc);
+ }

- ret = drm_bridge_attach(enc, pipes[i].bridge, NULL, 0);
- if (ret)
- return ret;
+ ret = drm_bridge_attach(enc, pipes[i].bridge[j], NULL, 0);
+ if (ret)
+ return ret;
+ }
+ num_encoders += pipes[i].num_bridges;
}

/* create overlay planes of the leftover planes */
--
2.38.1


2022-11-19 17:47:10

by Aradhya Bhatia

[permalink] [raw]
Subject: [PATCH v6 5/5] drm/tidss: Enable Dual and Duplicate Modes for OLDI

The AM625 DSS IP contains 2 OLDI TXes which can work together to enable 2
cloned displays of or even a single dual-link display with higher
resolutions like WUXGA (1920x1200@60fps) with a reduced OLDI clock
frequency.

Configure the necessary register to enable and disable the OLDI TXes
with required modes configurations.

Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/tidss/tidss_dispc.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index f26129fb1d8f..cf43de6216a5 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -1012,8 +1012,8 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
int count = 0;

/*
- * For the moment DUALMODESYNC, MASTERSLAVE, MODE, and SRC
- * bits of DISPC_VP_DSS_OLDI_CFG are set statically to 0.
+ * For the moment MASTERSLAVE, and SRC bits of DISPC_VP_DSS_OLDI_CFG are
+ * always set to 0.
*/

if (fmt->data_width == 24)
@@ -1030,6 +1030,26 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,

oldi_cfg |= BIT(0); /* ENABLE */

+ switch (dispc->oldi_mode) {
+ case OLDI_SINGLE_LINK_SINGLE_MODE:
+ /* All configuration is done for this mode. */
+ break;
+
+ case OLDI_SINGLE_LINK_CLONE_MODE:
+ oldi_cfg |= BIT(5); /* CLONE MODE */
+ break;
+
+ case OLDI_DUAL_LINK_MODE:
+ oldi_cfg |= BIT(11); /* DUALMODESYNC */
+ oldi_cfg |= BIT(3); /* data-mapping field also indicates dual-link mode */
+ break;
+
+ default:
+ dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
+ __func__);
+ return;
+ }
+
dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);

while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
--
2.38.1


2022-11-19 17:48:00

by Aradhya Bhatia

[permalink] [raw]
Subject: [PATCH v6 1/5] dt-bindings: display: ti,am65x-dss: Add support for am625 dss

The DSS controller on TI's AM625 SoC is an update from that on TI's
AM65X SoC. The former has an additional OLDI TX on its first video port
(VP0) that helps output cloned video or WUXGA (1920x1200@60fps)
resolution video output over a dual-link mode to reduce the required
OLDI clock output.

Add the new controller's compatible and a port property for the 2nd OLDI
TX (OLDI TX 1).

Signed-off-by: Aradhya Bhatia <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Reviewed-by: Rahul T R <[email protected]>
---
.../bindings/display/ti/ti,am65x-dss.yaml | 23 +++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index 5c7d2cbc4aac..55ec91f11577 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -19,7 +19,9 @@ description: |

properties:
compatible:
- const: ti,am65x-dss
+ enum:
+ - ti,am625-dss
+ - ti,am65x-dss

reg:
description:
@@ -80,13 +82,18 @@ properties:
port@0:
$ref: /schemas/graph.yaml#/properties/port
description:
- The DSS OLDI output port node form video port 1
+ The DSS OLDI output port node form video port 1 (OLDI TX 0).

port@1:
$ref: /schemas/graph.yaml#/properties/port
description:
The DSS DPI output port node from video port 2

+ port@2:
+ $ref: /schemas/graph.yaml#/properties/port
+ description:
+ The DSS OLDI output port node form video port 1 (OLDI TX 1).
+
ti,am65x-oldi-io-ctrl:
$ref: "/schemas/types.yaml#/definitions/phandle"
description:
@@ -102,6 +109,18 @@ properties:
Input memory (from main memory to dispc) bandwidth limit in
bytes per second

+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: ti,am65x-dss
+ then:
+ properties:
+ ports:
+ properties:
+ port@2: false
+
required:
- compatible
- reg
--
2.38.1


2022-11-19 17:52:11

by Aradhya Bhatia

[permalink] [raw]
Subject: [PATCH v6 2/5] drm/tidss: Add support for AM625 DSS

Add support for the DSS controller on TI's new AM625 SoC in the tidss
driver.

The first video port (VP0) in am625-dss can output OLDI signals through
2 OLDI TXes. A 3rd output port has been added with "DISPC_VP_OLDI" bus
type.

DSS controllers before AM625 had a 1 to 1 coupling between Videoports
and Output Ports. Since this stands no longer to be true for AM625 DSS,
this couppling has been separated with the introduction of output port
based variables. This will help address the addition of the 2nd OLDI TX
over VP0 as the 3rd output port.

Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/tidss/tidss_dispc.c | 80 ++++++++++++++++++++++++++---
drivers/gpu/drm/tidss/tidss_dispc.h | 15 ++++--
drivers/gpu/drm/tidss/tidss_drv.c | 1 +
drivers/gpu/drm/tidss/tidss_drv.h | 5 +-
drivers/gpu/drm/tidss/tidss_irq.h | 2 +-
drivers/gpu/drm/tidss/tidss_kms.c | 2 +-
6 files changed, 90 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index ad93acc9abd2..dbc6a5b617ca 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -93,10 +93,13 @@ const struct dispc_features dispc_k2g_feats = {
.common_regs = tidss_k2g_common_regs,

.num_vps = 1,
+ .num_output_ports = 1,
+ .oldi_supported = false,
.vp_name = { "vp1" },
.ovr_name = { "ovr1" },
.vpclk_name = { "vp1" },
.vp_bus_type = { DISPC_VP_DPI },
+ .output_port_bus_type = { DISPC_VP_DPI },

.vp_feat = { .color = {
.has_ctm = true,
@@ -168,10 +171,13 @@ const struct dispc_features dispc_am65x_feats = {
.common_regs = tidss_am65x_common_regs,

.num_vps = 2,
+ .num_output_ports = 2,
+ .oldi_supported = true,
.vp_name = { "vp1", "vp2" },
.ovr_name = { "ovr1", "ovr2" },
.vpclk_name = { "vp1", "vp2" },
.vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI },
+ .output_port_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI },

.vp_feat = { .color = {
.has_ctm = true,
@@ -257,12 +263,16 @@ const struct dispc_features dispc_j721e_feats = {
.common_regs = tidss_j721e_common_regs,

.num_vps = 4,
+ .num_output_ports = 4,
+ .oldi_supported = false,
.vp_name = { "vp1", "vp2", "vp3", "vp4" },
.ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" },
.vpclk_name = { "vp1", "vp2", "vp3", "vp4" },
/* Currently hard coded VP routing (see dispc_initial_config()) */
.vp_bus_type = { DISPC_VP_INTERNAL, DISPC_VP_DPI,
DISPC_VP_INTERNAL, DISPC_VP_DPI, },
+ .output_port_bus_type = { DISPC_VP_INTERNAL, DISPC_VP_DPI,
+ DISPC_VP_INTERNAL, DISPC_VP_DPI, },
.vp_feat = { .color = {
.has_ctm = true,
.gamma_size = 1024,
@@ -275,6 +285,59 @@ const struct dispc_features dispc_j721e_feats = {
.vid_order = { 1, 3, 0, 2 },
};

+const struct dispc_features dispc_am625_feats = {
+ .max_pclk_khz = {
+ [DISPC_VP_DPI] = 165000,
+ [DISPC_VP_OLDI] = 165000,
+ },
+
+ .scaling = {
+ .in_width_max_5tap_rgb = 1280,
+ .in_width_max_3tap_rgb = 2560,
+ .in_width_max_5tap_yuv = 2560,
+ .in_width_max_3tap_yuv = 4096,
+ .upscale_limit = 16,
+ .downscale_limit_5tap = 4,
+ .downscale_limit_3tap = 2,
+ /*
+ * The max supported pixel inc value is 255. The value
+ * of pixel inc is calculated like this: 1+(xinc-1)*bpp.
+ * The maximum bpp of all formats supported by the HW
+ * is 8. So the maximum supported xinc value is 32,
+ * because 1+(32-1)*8 < 255 < 1+(33-1)*4.
+ */
+ .xinc_max = 32,
+ },
+
+ .subrev = DISPC_AM625,
+
+ .common = "common",
+ .common_regs = tidss_am65x_common_regs,
+
+ .num_vps = 2,
+ /* note: the 3rd port is not representative of a 3rd pipeline */
+ .num_output_ports = 3,
+ .oldi_supported = true,
+ .vp_name = { "vp1", "vp2" },
+ .ovr_name = { "ovr1", "ovr2" },
+ .vpclk_name = { "vp1", "vp2" },
+ .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI },
+ .output_port_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI, DISPC_VP_OLDI },
+
+ .vp_feat = { .color = {
+ .has_ctm = true,
+ .gamma_size = 256,
+ .gamma_type = TIDSS_GAMMA_8BIT,
+ },
+ },
+
+ .num_planes = 2,
+ /* note: vid is plane_id 0 and vidl1 is plane_id 1 */
+ .vid_name = { "vid", "vidl1" },
+ .vid_lite = { false, true, },
+ .vid_order = { 1, 0 },
+};
+
static const u16 *dispc_common_regmap;

struct dss_vp_data {
@@ -287,12 +350,12 @@ struct dispc_device {

void __iomem *base_common;
void __iomem *base_vid[TIDSS_MAX_PLANES];
- void __iomem *base_ovr[TIDSS_MAX_PORTS];
- void __iomem *base_vp[TIDSS_MAX_PORTS];
+ void __iomem *base_ovr[TIDSS_MAX_VPS];
+ void __iomem *base_vp[TIDSS_MAX_VPS];

struct regmap *oldi_io_ctrl;

- struct clk *vp_clk[TIDSS_MAX_PORTS];
+ struct clk *vp_clk[TIDSS_MAX_VPS];

const struct dispc_features *feat;

@@ -300,7 +363,7 @@ struct dispc_device {

bool is_enabled;

- struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
+ struct dss_vp_data vp_data[TIDSS_MAX_VPS];

u32 *fourccs;
u32 num_fourccs;
@@ -778,6 +841,7 @@ dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc)
return dispc_k2g_read_and_clear_irqstatus(dispc);
case DISPC_AM65X:
case DISPC_J721E:
+ case DISPC_AM625:
return dispc_k3_read_and_clear_irqstatus(dispc);
default:
WARN_ON(1);
@@ -793,6 +857,7 @@ void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
break;
case DISPC_AM65X:
case DISPC_J721E:
+ case DISPC_AM625:
dispc_k3_set_irqenable(dispc, mask);
break;
default:
@@ -1116,7 +1181,7 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
const struct drm_display_mode *mode)
{
u32 hsw, hfp, hbp, vsw, vfp, vbp;
- enum dispc_vp_bus_type bus_type;
+ enum dispc_bus_type bus_type;
int max_pclk;

bus_type = dispc->feat->vp_bus_type[hw_videoport];
@@ -1282,6 +1347,7 @@ void dispc_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane,
x, y, layer);
break;
case DISPC_AM65X:
+ case DISPC_AM625:
dispc_am65x_ovr_set_plane(dispc, hw_plane, hw_videoport,
x, y, layer);
break;
@@ -2205,6 +2271,7 @@ static void dispc_plane_init(struct dispc_device *dispc)
break;
case DISPC_AM65X:
case DISPC_J721E:
+ case DISPC_AM625:
dispc_k3_plane_init(dispc);
break;
default:
@@ -2310,6 +2377,7 @@ static void dispc_vp_write_gamma_table(struct dispc_device *dispc,
dispc_k2g_vp_write_gamma_table(dispc, hw_videoport);
break;
case DISPC_AM65X:
+ case DISPC_AM625:
dispc_am65x_vp_write_gamma_table(dispc, hw_videoport);
break;
case DISPC_J721E:
@@ -2583,7 +2651,7 @@ int dispc_runtime_resume(struct dispc_device *dispc)
REG_GET(dispc, DSS_SYSSTATUS, 2, 2),
REG_GET(dispc, DSS_SYSSTATUS, 3, 3));

- if (dispc->feat->subrev == DISPC_AM65X)
+ if (dispc->feat->oldi_supported)
dev_dbg(dispc->dev, "OLDI RESETDONE %d,%d,%d\n",
REG_GET(dispc, DSS_SYSSTATUS, 5, 5),
REG_GET(dispc, DSS_SYSSTATUS, 6, 6),
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index e49432f0abf5..51db500590ee 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -50,7 +50,7 @@ struct dispc_errata {
bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */
};

-enum dispc_vp_bus_type {
+enum dispc_bus_type {
DISPC_VP_DPI, /* DPI output */
DISPC_VP_OLDI, /* OLDI (LVDS) output */
DISPC_VP_INTERNAL, /* SoC internal routing */
@@ -61,6 +61,7 @@ enum dispc_dss_subrevision {
DISPC_K2G,
DISPC_AM65X,
DISPC_J721E,
+ DISPC_AM625,
};

struct dispc_features {
@@ -74,10 +75,13 @@ struct dispc_features {
const char *common;
const u16 *common_regs;
u32 num_vps;
- const char *vp_name[TIDSS_MAX_PORTS]; /* Should match dt reg names */
- const char *ovr_name[TIDSS_MAX_PORTS]; /* Should match dt reg names */
- const char *vpclk_name[TIDSS_MAX_PORTS]; /* Should match dt clk names */
- const enum dispc_vp_bus_type vp_bus_type[TIDSS_MAX_PORTS];
+ u32 num_output_ports;
+ bool oldi_supported;
+ const char *vp_name[TIDSS_MAX_VPS]; /* Should match dt reg names */
+ const char *ovr_name[TIDSS_MAX_VPS]; /* Should match dt reg names */
+ const char *vpclk_name[TIDSS_MAX_VPS]; /* Should match dt clk names */
+ const enum dispc_bus_type vp_bus_type[TIDSS_MAX_VPS];
+ const enum dispc_bus_type output_port_bus_type[TIDSS_MAX_OUTPUT_PORTS];
struct tidss_vp_feat vp_feat;
u32 num_planes;
const char *vid_name[TIDSS_MAX_PLANES]; /* Should match dt reg names */
@@ -88,6 +92,7 @@ struct dispc_features {
extern const struct dispc_features dispc_k2g_feats;
extern const struct dispc_features dispc_am65x_feats;
extern const struct dispc_features dispc_j721e_feats;
+extern const struct dispc_features dispc_am625_feats;

void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask);
dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc);
diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index 15cd9b91b7e2..c5e119828823 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -235,6 +235,7 @@ static const struct of_device_id tidss_of_table[] = {
{ .compatible = "ti,k2g-dss", .data = &dispc_k2g_feats, },
{ .compatible = "ti,am65x-dss", .data = &dispc_am65x_feats, },
{ .compatible = "ti,j721e-dss", .data = &dispc_j721e_feats, },
+ { .compatible = "ti,am625-dss", .data = &dispc_am625_feats, },
{ }
};

diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
index d7f27b0b0315..0ce7ee5ccd5b 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -9,8 +9,9 @@

#include <linux/spinlock.h>

-#define TIDSS_MAX_PORTS 4
+#define TIDSS_MAX_VPS 4
#define TIDSS_MAX_PLANES 4
+#define TIDSS_MAX_OUTPUT_PORTS 4

typedef u32 dispc_irq_t;

@@ -22,7 +23,7 @@ struct tidss_device {
struct dispc_device *dispc;

unsigned int num_crtcs;
- struct drm_crtc *crtcs[TIDSS_MAX_PORTS];
+ struct drm_crtc *crtcs[TIDSS_MAX_VPS];

unsigned int num_planes;
struct drm_plane *planes[TIDSS_MAX_PLANES];
diff --git a/drivers/gpu/drm/tidss/tidss_irq.h b/drivers/gpu/drm/tidss/tidss_irq.h
index b512614d5863..a753f5e3ce15 100644
--- a/drivers/gpu/drm/tidss/tidss_irq.h
+++ b/drivers/gpu/drm/tidss/tidss_irq.h
@@ -35,7 +35,7 @@

#define DSS_IRQ_VP_BIT_N(ch, bit) (4 + 4 * (ch) + (bit))
#define DSS_IRQ_PLANE_BIT_N(plane, bit) \
- (DSS_IRQ_VP_BIT_N(TIDSS_MAX_PORTS, 0) + 1 * (plane) + (bit))
+ (DSS_IRQ_VP_BIT_N(TIDSS_MAX_VPS, 0) + 1 * (plane) + (bit))

#define DSS_IRQ_VP_BIT(ch, bit) BIT(DSS_IRQ_VP_BIT_N((ch), (bit)))
#define DSS_IRQ_PLANE_BIT(plane, bit) \
diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
index afb2879980c6..fc9c4eefd31d 100644
--- a/drivers/gpu/drm/tidss/tidss_kms.c
+++ b/drivers/gpu/drm/tidss/tidss_kms.c
@@ -123,7 +123,7 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
u32 max_vps = feat->num_vps;
u32 max_planes = feat->num_planes;

- struct pipe pipes[TIDSS_MAX_PORTS];
+ struct pipe pipes[TIDSS_MAX_VPS];
u32 num_pipes = 0;
u32 crtc_mask;

--
2.38.1


2022-11-19 18:19:59

by Aradhya Bhatia

[permalink] [raw]
Subject: [PATCH v6 4/5] drm/tidss: Add IO CTRL and Power support for OLDI TX in am625

The ctrl mmr module of the AM625 is different from the AM65X SoC. Thus
the ctrl mmr registers that supported the OLDI TX power have become
different in AM625 SoC.

Add IO CTRL support and control the OLDI TX power for AM625.

Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/tidss/tidss_dispc.c | 55 ++++++++++++++++++------
drivers/gpu/drm/tidss/tidss_dispc_regs.h | 37 +++++++++++-----
2 files changed, 70 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 472226a83251..f26129fb1d8f 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -930,21 +930,52 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport,

static void dispc_oldi_tx_power(struct dispc_device *dispc, bool power)
{
- u32 val = power ? 0 : OLDI_PWRDN_TX;
+ u32 val;

if (WARN_ON(!dispc->oldi_io_ctrl))
return;

- regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT0_IO_CTRL,
- OLDI_PWRDN_TX, val);
- regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT1_IO_CTRL,
- OLDI_PWRDN_TX, val);
- regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT2_IO_CTRL,
- OLDI_PWRDN_TX, val);
- regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT3_IO_CTRL,
- OLDI_PWRDN_TX, val);
- regmap_update_bits(dispc->oldi_io_ctrl, OLDI_CLK_IO_CTRL,
- OLDI_PWRDN_TX, val);
+ if (dispc->feat->subrev == DISPC_AM65X) {
+ val = power ? 0 : AM65X_OLDI_PWRDN_TX;
+
+ regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT0_IO_CTRL,
+ AM65X_OLDI_PWRDN_TX, val);
+ regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT1_IO_CTRL,
+ AM65X_OLDI_PWRDN_TX, val);
+ regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT2_IO_CTRL,
+ AM65X_OLDI_PWRDN_TX, val);
+ regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT3_IO_CTRL,
+ AM65X_OLDI_PWRDN_TX, val);
+ regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_CLK_IO_CTRL,
+ AM65X_OLDI_PWRDN_TX, val);
+
+ } else if (dispc->feat->subrev == DISPC_AM625) {
+ if (power) {
+ switch (dispc->oldi_mode) {
+ case OLDI_SINGLE_LINK_SINGLE_MODE:
+ /* Power down OLDI TX 1 */
+ val = AM625_OLDI1_PWRDN_TX;
+ break;
+
+ case OLDI_SINGLE_LINK_CLONE_MODE:
+ case OLDI_DUAL_LINK_MODE:
+ /* No Power down */
+ val = 0;
+ break;
+
+ default:
+ /* Power down both the OLDI TXes */
+ val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX;
+ break;
+ }
+ } else {
+ /* Power down both the OLDI TXes */
+ val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX;
+ }
+
+ regmap_update_bits(dispc->oldi_io_ctrl, AM625_OLDI_PD_CTRL,
+ AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX, val);
+ }
}

static void dispc_set_num_datalines(struct dispc_device *dispc,
@@ -2841,7 +2872,7 @@ int dispc_init(struct tidss_device *tidss)
dispc->vp_data[i].gamma_table = gamma_table;
}

- if (feat->subrev == DISPC_AM65X) {
+ if (feat->oldi_supported) {
r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
if (r)
return r;
diff --git a/drivers/gpu/drm/tidss/tidss_dispc_regs.h b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
index 13feedfe5d6d..ccc4f29b7f1b 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
@@ -227,17 +227,34 @@ enum dispc_common_regs {
#define DISPC_VP_DSS_DMA_THREADSIZE_STATUS 0x174 /* J721E */

/*
- * OLDI IO_CTRL register offsets. On AM654 the registers are found
- * from CTRL_MMR0, there the syscon regmap should map 0x14 bytes from
- * CTRLMMR0P1_OLDI_DAT0_IO_CTRL to CTRLMMR0P1_OLDI_CLK_IO_CTRL
- * register range.
+ * OLDI IO and PD CTRL register offsets.
+ * These registers are found in the CTRL_MMR0, where the syscon regmap should map
+ *
+ * 1. 0x14 bytes from CTRLMMR0P1_OLDI_DAT0_IO_CTRL to CTRLMMR0P1_OLDI_CLK_IO_CTRL
+ * register range for the AM65X DSS, and
+ *
+ * 2. 0x200 bytes from OLDI0_DAT0_IO_CTRL to OLDI_LB_CTRL register range for the
+ * AM625 DSS.
*/
-#define OLDI_DAT0_IO_CTRL 0x00
-#define OLDI_DAT1_IO_CTRL 0x04
-#define OLDI_DAT2_IO_CTRL 0x08
-#define OLDI_DAT3_IO_CTRL 0x0C
-#define OLDI_CLK_IO_CTRL 0x10

-#define OLDI_PWRDN_TX BIT(8)
+/* -- For AM65X OLDI TX -- */
+/* Register offsets */
+#define AM65X_OLDI_DAT0_IO_CTRL 0x00
+#define AM65X_OLDI_DAT1_IO_CTRL 0x04
+#define AM65X_OLDI_DAT2_IO_CTRL 0x08
+#define AM65X_OLDI_DAT3_IO_CTRL 0x0C
+#define AM65X_OLDI_CLK_IO_CTRL 0x10
+
+/* Power control bits */
+#define AM65X_OLDI_PWRDN_TX BIT(8)
+
+/* -- For AM625 OLDI TX -- */
+/* Register offsets */
+#define AM625_OLDI_PD_CTRL 0x100
+#define AM625_OLDI_LB_CTRL 0x104
+
+/* Power control bits */
+#define AM625_OLDI0_PWRDN_TX BIT(0)
+#define AM625_OLDI1_PWRDN_TX BIT(1)

#endif /* __TIDSS_DISPC_REGS_H */
--
2.38.1


2022-12-20 13:05:08

by Tomi Valkeinen

[permalink] [raw]
Subject: Re: [PATCH v6 4/5] drm/tidss: Add IO CTRL and Power support for OLDI TX in am625

Hi,

On 19/11/2022 19:30, Aradhya Bhatia wrote:
> The ctrl mmr module of the AM625 is different from the AM65X SoC. Thus
> the ctrl mmr registers that supported the OLDI TX power have become
> different in AM625 SoC.
>
> Add IO CTRL support and control the OLDI TX power for AM625.
>
> Signed-off-by: Aradhya Bhatia <[email protected]>
> ---
> drivers/gpu/drm/tidss/tidss_dispc.c | 55 ++++++++++++++++++------
> drivers/gpu/drm/tidss/tidss_dispc_regs.h | 37 +++++++++++-----
> 2 files changed, 70 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 472226a83251..f26129fb1d8f 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -930,21 +930,52 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport,
>
> static void dispc_oldi_tx_power(struct dispc_device *dispc, bool power)
> {
> - u32 val = power ? 0 : OLDI_PWRDN_TX;
> + u32 val;
>
> if (WARN_ON(!dispc->oldi_io_ctrl))
> return;
>
> - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT0_IO_CTRL,
> - OLDI_PWRDN_TX, val);
> - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT1_IO_CTRL,
> - OLDI_PWRDN_TX, val);
> - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT2_IO_CTRL,
> - OLDI_PWRDN_TX, val);
> - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT3_IO_CTRL,
> - OLDI_PWRDN_TX, val);
> - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_CLK_IO_CTRL,
> - OLDI_PWRDN_TX, val);
> + if (dispc->feat->subrev == DISPC_AM65X) {
> + val = power ? 0 : AM65X_OLDI_PWRDN_TX;
> +
> + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT0_IO_CTRL,
> + AM65X_OLDI_PWRDN_TX, val);
> + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT1_IO_CTRL,
> + AM65X_OLDI_PWRDN_TX, val);
> + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT2_IO_CTRL,
> + AM65X_OLDI_PWRDN_TX, val);
> + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT3_IO_CTRL,
> + AM65X_OLDI_PWRDN_TX, val);
> + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_CLK_IO_CTRL,
> + AM65X_OLDI_PWRDN_TX, val);
> +
> + } else if (dispc->feat->subrev == DISPC_AM625) {
> + if (power) {
> + switch (dispc->oldi_mode) {
> + case OLDI_SINGLE_LINK_SINGLE_MODE:
> + /* Power down OLDI TX 1 */
> + val = AM625_OLDI1_PWRDN_TX;
> + break;
> +
> + case OLDI_SINGLE_LINK_CLONE_MODE:
> + case OLDI_DUAL_LINK_MODE:
> + /* No Power down */
> + val = 0;
> + break;
> +
> + default:
> + /* Power down both the OLDI TXes */
> + val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX;
> + break;
> + }
> + } else {
> + /* Power down both the OLDI TXes */
> + val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX;
> + }
> +
> + regmap_update_bits(dispc->oldi_io_ctrl, AM625_OLDI_PD_CTRL,
> + AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX, val);
> + }
> }
>
> static void dispc_set_num_datalines(struct dispc_device *dispc,
> @@ -2841,7 +2872,7 @@ int dispc_init(struct tidss_device *tidss)
> dispc->vp_data[i].gamma_table = gamma_table;
> }
>
> - if (feat->subrev == DISPC_AM65X) {
> + if (feat->oldi_supported) {
> r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
> if (r)
> return r;

I think it makes more sense to test the SoC version here, rather than
the generic "oldi_supported". And if the same function is used for
am625, maybe rename the func to "_am6xx_".

Other than that:

Reviewed-by: Tomi Valkeinen <[email protected]>

Tomi

2022-12-20 13:13:52

by Tomi Valkeinen

[permalink] [raw]
Subject: Re: [PATCH v6 5/5] drm/tidss: Enable Dual and Duplicate Modes for OLDI

On 19/11/2022 19:30, Aradhya Bhatia wrote:
> The AM625 DSS IP contains 2 OLDI TXes which can work together to enable 2
> cloned displays of or even a single dual-link display with higher
> resolutions like WUXGA (1920x1200@60fps) with a reduced OLDI clock
> frequency.
>
> Configure the necessary register to enable and disable the OLDI TXes
> with required modes configurations.
>
> Signed-off-by: Aradhya Bhatia <[email protected]>
> ---
> drivers/gpu/drm/tidss/tidss_dispc.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index f26129fb1d8f..cf43de6216a5 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -1012,8 +1012,8 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
> int count = 0;
>
> /*
> - * For the moment DUALMODESYNC, MASTERSLAVE, MODE, and SRC
> - * bits of DISPC_VP_DSS_OLDI_CFG are set statically to 0.
> + * For the moment MASTERSLAVE, and SRC bits of DISPC_VP_DSS_OLDI_CFG are
> + * always set to 0.
> */
>
> if (fmt->data_width == 24)
> @@ -1030,6 +1030,26 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
>
> oldi_cfg |= BIT(0); /* ENABLE */
>
> + switch (dispc->oldi_mode) {
> + case OLDI_SINGLE_LINK_SINGLE_MODE:
> + /* All configuration is done for this mode. */
> + break;
> +
> + case OLDI_SINGLE_LINK_CLONE_MODE:
> + oldi_cfg |= BIT(5); /* CLONE MODE */
> + break;
> +
> + case OLDI_DUAL_LINK_MODE:
> + oldi_cfg |= BIT(11); /* DUALMODESYNC */
> + oldi_cfg |= BIT(3); /* data-mapping field also indicates dual-link mode */
> + break;
> +
> + default:
> + dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
> + __func__);
> + return;
> + }
> +
> dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>
> while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&

Reviewed-by: Tomi Valkeinen <[email protected]>

Tomi

2022-12-20 13:18:35

by Tomi Valkeinen

[permalink] [raw]
Subject: Re: [PATCH v6 3/5] drm/tidss: Add support to configure OLDI mode for am625-dss.

Hi,

On 19/11/2022 19:30, Aradhya Bhatia wrote:
> The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal.
> These can be configured to support the following modes:
>
> 1. OLDI_SINGLE_LINK_SINGLE_MODE
> Single Output over OLDI 0.
> +------+ +---------+ +-------+
> | | | | | |
> | CRTC +------->+ ENCODER +----->| PANEL |
> | | | | | |
> +------+ +---------+ +-------+
>
> 2. OLDI_SINGLE_LINK_CLONE_MODE
> Duplicate Output over OLDI 0 and 1.
> +------+ +---------+ +-------+
> | | | | | |
> | CRTC +---+--->| ENCODER +----->| PANEL |
> | | | | | | |
> +------+ | +---------+ +-------+
> |
> | +---------+ +-------+
> | | | | |
> +--->| ENCODER +----->| PANEL |
> | | | |
> +---------+ +-------+
>
> 3. OLDI_DUAL_LINK_MODE
> Combined Output over OLDI 0 and 1.
> +------+ +---------+ +-------+
> | | | +----->| |
> | CRTC +------->+ ENCODER | | PANEL |
> | | | +----->| |
> +------+ +---------+ +-------+
>
> Following the above pathways for different modes, 2 encoder/panel-bridge
> pipes get created for clone mode, and 1 pipe in cases of single link and
> dual link mode.
>
> Add support for confguring the OLDI modes using OF and LVDS DRM helper
> functions.
>
> Signed-off-by: Aradhya Bhatia <[email protected]>
> ---
> drivers/gpu/drm/tidss/tidss_dispc.c | 12 ++
> drivers/gpu/drm/tidss/tidss_dispc.h | 9 ++
> drivers/gpu/drm/tidss/tidss_drv.h | 3 +
> drivers/gpu/drm/tidss/tidss_encoder.c | 4 +-
> drivers/gpu/drm/tidss/tidss_encoder.h | 3 +-
> drivers/gpu/drm/tidss/tidss_kms.c | 188 +++++++++++++++++++++++---
> 6 files changed, 198 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index dbc6a5b617ca..472226a83251 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -365,6 +365,8 @@ struct dispc_device {
>
> struct dss_vp_data vp_data[TIDSS_MAX_VPS];
>
> + enum dispc_oldi_modes oldi_mode;
> +
> u32 *fourccs;
> u32 num_fourccs;
>
> @@ -1967,6 +1969,16 @@ const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len)
> return dispc->fourccs;
> }
>
> +int dispc_set_oldi_mode(struct dispc_device *dispc,
> + enum dispc_oldi_modes oldi_mode)
> +{
> + WARN_ON(!dispc);

This feels unnecessary. Is there even a theoretical case where we could
get dispc == NULL?

> +
> + dispc->oldi_mode = oldi_mode;
> +
> + return 0;

This function could as well be void function.

> +}
> +
> static s32 pixinc(int pixels, u8 ps)
> {
> if (pixels == 1)
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
> index 51db500590ee..e76a7599b544 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
> @@ -64,6 +64,14 @@ enum dispc_dss_subrevision {
> DISPC_AM625,
> };
>
> +enum dispc_oldi_modes {
> + OLDI_MODE_OFF, /* OLDI turned off / tied off in IP. */
> + OLDI_SINGLE_LINK_SINGLE_MODE, /* Single Output over OLDI 0. */
> + OLDI_SINGLE_LINK_CLONE_MODE, /* Duplicate Output over OLDI 0 and 1. */
> + OLDI_DUAL_LINK_MODE, /* Combined Output over OLDI 0 and 1. */
> + OLDI_MODE_UNSUPPORTED, /* Unsupported OLDI Mode */
> +};

What is the difference with MODE_OFF and MODE_UNSUPPORTED? Is
MODE_UNSUPPORTED for cases where, e.g., the DT setup is wrong and the
driver should return an error? The code doesn't quite do that, it prints
an error but then continues.

> struct dispc_features {
> int min_pclk_khz;
> int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
> @@ -133,6 +141,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
> u32 hw_videoport);
> int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable);
> const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len);
> +int dispc_set_oldi_mode(struct dispc_device *dispc, enum dispc_oldi_modes oldi_mode);
>
> int dispc_init(struct tidss_device *tidss);
> void dispc_remove(struct tidss_device *tidss);
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
> index 0ce7ee5ccd5b..58892f065c16 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.h
> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
> @@ -13,6 +13,9 @@
> #define TIDSS_MAX_PLANES 4
> #define TIDSS_MAX_OUTPUT_PORTS 4
>
> +/* For AM625-DSS with 2 OLDI TXes */
> +#define TIDSS_MAX_BRIDGES_PER_PIPE 2
> +
> typedef u32 dispc_irq_t;
>
> struct tidss_device {
> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
> index e278a9c89476..141383ec4045 100644
> --- a/drivers/gpu/drm/tidss/tidss_encoder.c
> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
> @@ -70,7 +70,8 @@ static const struct drm_encoder_funcs encoder_funcs = {
> };
>
> struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
> - u32 encoder_type, u32 possible_crtcs)
> + u32 encoder_type, u32 possible_crtcs,
> + u32 possible_clones)
> {
> struct drm_encoder *enc;
> int ret;
> @@ -80,6 +81,7 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
> return ERR_PTR(-ENOMEM);
>
> enc->possible_crtcs = possible_crtcs;
> + enc->possible_clones = possible_clones;
>
> ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs,
> encoder_type, NULL);
> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h b/drivers/gpu/drm/tidss/tidss_encoder.h
> index ace877c0e0fd..01c62ba3ef16 100644
> --- a/drivers/gpu/drm/tidss/tidss_encoder.h
> +++ b/drivers/gpu/drm/tidss/tidss_encoder.h
> @@ -12,6 +12,7 @@
> struct tidss_device;
>
> struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
> - u32 encoder_type, u32 possible_crtcs);
> + u32 encoder_type, u32 possible_crtcs,
> + u32 possible_clones);
>
> #endif
> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> index fc9c4eefd31d..8ae321f02197 100644
> --- a/drivers/gpu/drm/tidss/tidss_kms.c
> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
> @@ -106,30 +106,115 @@ static const struct drm_mode_config_funcs mode_config_funcs = {
> .atomic_commit = drm_atomic_helper_commit,
> };
>
> +static enum dispc_oldi_modes tidss_get_oldi_mode(struct device_node *oldi0_port,
> + struct device_node *oldi1_port)
> +{
> + int pixel_order;
> +
> + if (!(oldi0_port || oldi1_port)) {
> + /* Keep OLDI TXes off if neither OLDI port is present. */
> + return OLDI_MODE_OFF;
> + } else if (oldi0_port && !oldi1_port) {
> + /*
> + * OLDI0 port found, but not OLDI1 port. Setting single
> + * link, single mode output.
> + */
> + return OLDI_SINGLE_LINK_SINGLE_MODE;
> + } else if (!oldi0_port && oldi1_port) {
> + /*
> + * The 2nd OLDI TX cannot be operated alone. This use case is
> + * not supported in the HW. Since the pins for OLDIs 0 and 1 are
> + * separate, one could theoretically set a clone mode over OLDIs
> + * 0 and 1 and just simply not use the OLDI 0. This is a hacky
> + * way to enable only OLDI TX 1 and hence is not officially
> + * supported.
> + */
> + return OLDI_MODE_UNSUPPORTED;

If OLDI_MODE_UNSUPPORTED is supposed to result in an error, maybe you
could print the error here (and possibly in the default case below), and
then, in the caller, just return with an error code.

> + }
> +
> + /*
> + * OLDI Ports found for both the OLDI TXes. The DSS is to be configured
> + * in either Dual Link or Clone Mode.
> + */
> + pixel_order = drm_of_lvds_get_dual_link_pixel_order(oldi0_port,
> + oldi1_port);
> + switch (pixel_order) {
> + case -EINVAL:
> + /*
> + * The dual link properties were not found in at least one of
> + * the sink nodes. Since 2 OLDI ports are present in the DT, it
> + * can be safely assumed that the required configuration is
> + * Clone Mode.
> + */
> + return OLDI_SINGLE_LINK_CLONE_MODE;
> +
> + case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
> + /*
> + * Note that the OLDI TX 0 transmits the odd set of pixels while
> + * the OLDI TX 1 transmits the even set. This is a fixed
> + * configuration in the IP and an cannot be change vis SW. These
> + * properties have been used to merely identify if a Dual Link
> + * configuration is required. Swapping this property in the panel
> + * port DT nodes will not make any difference.
> + */
> + pr_warn("EVEN-ODD config for dual-link sinks is not supported in HW. Switching to ODD-EVEN.\n");

Please use dev_warn() instead, so that it will be clear where the print
comes from.

In any case, isn't this an error? Do you really want to accept the wrong
pixel order?

> + fallthrough;
> +
> + case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
> + return OLDI_DUAL_LINK_MODE;
> +
> + default:
> + return OLDI_MODE_OFF;

When do we get here? Shouldn't this be OLDI_MODE_UNSUPPORTED?

> + }
> +}
> +
> static int tidss_dispc_modeset_init(struct tidss_device *tidss)
> {
> struct device *dev = tidss->dev;
> unsigned int fourccs_len;
> const u32 *fourccs = dispc_plane_formats(tidss->dispc, &fourccs_len);
> - unsigned int i;
> + unsigned int i, j;
>
> struct pipe {
> u32 hw_videoport;
> - struct drm_bridge *bridge;
> + struct drm_bridge *bridge[TIDSS_MAX_BRIDGES_PER_PIPE];
> u32 enc_type;
> + u32 num_bridges;
> };
>
> const struct dispc_features *feat = tidss->feat;
> - u32 max_vps = feat->num_vps;
> + u32 output_ports = feat->num_output_ports;
> u32 max_planes = feat->num_planes;
>
> - struct pipe pipes[TIDSS_MAX_VPS];
> + struct pipe pipes[TIDSS_MAX_VPS] = {0};
> +
> u32 num_pipes = 0;
> u32 crtc_mask;
> + u32 portnum_oldi0 = 0, portnum_oldi1 = 2;

These two are bit of hacks. First, they should be const, or maybe
defines. If they're const, they can be inside the block below.

And they're very much tied to the HW in question, so just having
hardcoded values here inside a function without any mention of the
situation is not good.

Doing this in a generic and future proof manner is... challenging. So I
think using the hardcoded port numbers is fine. But they are only ok for
the two AM6xx SoCs we have now so the 'oldi_supported' is not very good
fit. In fact, it might be good to drop 'oldi_supported' altogether, and
just check for the SoC versions instead, as (with a quick glance), all
the 'oldi_supported' checks are really SoC specific.

This also again brings up a thing that rubs me the wrong way: the new
OLDI port is port 2. I really think that on AM62x, the two OLDI ports
should be 0 and 1, and the DPI should be 2. Would we need a new
dt-binding doc for that, or could it still be described in the same doc?
Would that change cause changes elsewhere in the dss driver?

> + enum dispc_oldi_modes oldi_mode = OLDI_MODE_OFF;
> + u32 num_oldi = 0;
> + u32 oldi_pipe_index = 0;
> + u32 num_encoders = 0;
> +
> + if (feat->oldi_supported) {
> + struct device_node *oldi0_port, *oldi1_port;
> +
> + oldi0_port = of_graph_get_port_by_id(dev->of_node,
> + portnum_oldi0);
> + oldi1_port = of_graph_get_port_by_id(dev->of_node,
> + portnum_oldi1);
> +
> + oldi_mode = tidss_get_oldi_mode(oldi0_port, oldi1_port);
> +
> + of_node_put(oldi0_port);
> + of_node_put(oldi1_port);
> +
> + dispc_set_oldi_mode(tidss->dispc, oldi_mode);
> + }
>
> /* first find all the connected panels & bridges */
>
> - for (i = 0; i < max_vps; i++) {
> + for (i = 0; i < output_ports; i++) {
> struct drm_panel *panel;
> struct drm_bridge *bridge;
> u32 enc_type = DRM_MODE_ENCODER_NONE;
> @@ -145,16 +230,24 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
> return ret;
> }
>
> + if (feat->output_port_bus_type[i] == DISPC_VP_OLDI &&
> + oldi_mode == OLDI_MODE_UNSUPPORTED) {
> + dev_err(dev,
> + "Single Mode over OLDI 1 is not supported in HW. Keeping OLDI off.\n");
> + continue;

Should we error out here?

> + }
> +
> if (panel) {
> u32 conn_type;
>
> dev_dbg(dev, "Setting up panel for port %d\n", i);
>
> - switch (feat->vp_bus_type[i]) {
> + switch (feat->output_port_bus_type[i]) {
> case DISPC_VP_OLDI:
> enc_type = DRM_MODE_ENCODER_LVDS;
> conn_type = DRM_MODE_CONNECTOR_LVDS;
> break;
> +
> case DISPC_VP_DPI:
> enc_type = DRM_MODE_ENCODER_DPI;
> conn_type = DRM_MODE_CONNECTOR_DPI;
> @@ -172,6 +265,17 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
> return -EINVAL;
> }
>
> + /*
> + * If the 2nd OLDI port is discovered connected to a panel
> + * which is not to be connected in the Clone Mode then a
> + * bridge is not required because the detected port is the
> + * 2nd port for the previously connected panel.
> + */
> + if (feat->output_port_bus_type[i] == DISPC_VP_OLDI &&
> + oldi_mode != OLDI_SINGLE_LINK_CLONE_MODE &&

Hmm, shouldn't this be oldi_mode == OLDI_DUAL_LINK_MODE? Or rather,
shouldn't we test here if this is the second oldi display, and if so
which oldi-mode are we using. If dual-link, break. If clone, continue.
Otherwise, error.

> + num_oldi)
> + break;
> +
> bridge = devm_drm_panel_bridge_add(dev, panel);
> if (IS_ERR(bridge)) {
> dev_err(dev,
> @@ -181,10 +285,47 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
> }
> }
>
> - pipes[num_pipes].hw_videoport = i;
> - pipes[num_pipes].bridge = bridge;
> - pipes[num_pipes].enc_type = enc_type;
> - num_pipes++;
> + if (feat->output_port_bus_type[i] == DISPC_VP_OLDI) {
> + if (++num_oldi == 1) {
> + /* Setting up pipe parameters when 1st OLDI port is detected */
> +
> + pipes[num_pipes].hw_videoport = i;
> + pipes[num_pipes].enc_type = enc_type;
> +
> + /*
> + * Saving the pipe index in case its required for
> + * 2nd OLDI Port.
> + */
> + oldi_pipe_index = num_pipes;
> +
> + /*
> + * No additional pipe is required for the 2nd OLDI
> + * port, because the 2nd Encoder -> Bridge connection
> + * is the part of the first OLDI Port pipe.
> + *
> + * num_pipes will only be incremented when the first
> + * OLDI port is discovered.
> + */
> + num_pipes++;
> + }
> +
> + /*
> + * Bridge is required to be added only if the detected
> + * port is the first OLDI port or a subsequent port in
> + * Clone Mode.
> + */
> + if (oldi_mode == OLDI_SINGLE_LINK_CLONE_MODE ||
> + num_oldi == 1) {
> + pipes[oldi_pipe_index].bridge[num_oldi - 1] = bridge;
> + pipes[oldi_pipe_index].num_bridges++;
> + }
> + } else {
> + pipes[num_pipes].hw_videoport = i;
> + pipes[num_pipes].bridge[0] = bridge;
> + pipes[num_pipes].num_bridges++;
> + pipes[num_pipes].enc_type = enc_type;
> + num_pipes++;
> + }
> }
>
> /* all planes can be on any crtc */
> @@ -196,6 +337,7 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
> struct tidss_plane *tplane;
> struct tidss_crtc *tcrtc;
> struct drm_encoder *enc;
> + u32 possible_clones = 0;
> u32 hw_plane_id = feat->vid_order[tidss->num_planes];
> int ret;
>
> @@ -218,16 +360,24 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>
> tidss->crtcs[tidss->num_crtcs++] = &tcrtc->crtc;
>
> - enc = tidss_encoder_create(tidss, pipes[i].enc_type,
> - 1 << tcrtc->crtc.index);
> - if (IS_ERR(enc)) {
> - dev_err(tidss->dev, "encoder create failed\n");
> - return PTR_ERR(enc);
> - }
> + for (j = 0; j < pipes[i].num_bridges; j++) {
> + if (pipes[i].num_bridges > 1)
> + possible_clones = (((1 << pipes[i].num_bridges) - 1)
> + << num_encoders);

I think the above possible_clones assignment can be outside the for loop.

> +
> + enc = tidss_encoder_create(tidss, pipes[i].enc_type,
> + 1 << tcrtc->crtc.index,
> + possible_clones);
> + if (IS_ERR(enc)) {
> + dev_err(tidss->dev, "encoder create failed\n");
> + return PTR_ERR(enc);
> + }
>
> - ret = drm_bridge_attach(enc, pipes[i].bridge, NULL, 0);
> - if (ret)
> - return ret;
> + ret = drm_bridge_attach(enc, pipes[i].bridge[j], NULL, 0);
> + if (ret)
> + return ret;
> + }
> + num_encoders += pipes[i].num_bridges;
> }
>
> /* create overlay planes of the leftover planes */

Tomi

2022-12-20 13:19:12

by Tomi Valkeinen

[permalink] [raw]
Subject: Re: [PATCH v6 2/5] drm/tidss: Add support for AM625 DSS

Hi,

On 19/11/2022 19:30, Aradhya Bhatia wrote:
> Add support for the DSS controller on TI's new AM625 SoC in the tidss
> driver.
>
> The first video port (VP0) in am625-dss can output OLDI signals through
> 2 OLDI TXes. A 3rd output port has been added with "DISPC_VP_OLDI" bus
> type.
>
> DSS controllers before AM625 had a 1 to 1 coupling between Videoports
> and Output Ports. Since this stands no longer to be true for AM625 DSS,
> this couppling has been separated with the introduction of output port
> based variables. This will help address the addition of the 2nd OLDI TX
> over VP0 as the 3rd output port.

This patch does three things:
- Renames "port" to "vp" where applicable
- Adds output ports
- Adds AM625

I think at least the AM625 parts should be a separate patch, but
preferably all three would be separate patches.

> Signed-off-by: Aradhya Bhatia <[email protected]>
> ---
> drivers/gpu/drm/tidss/tidss_dispc.c | 80 ++++++++++++++++++++++++++---
> drivers/gpu/drm/tidss/tidss_dispc.h | 15 ++++--
> drivers/gpu/drm/tidss/tidss_drv.c | 1 +
> drivers/gpu/drm/tidss/tidss_drv.h | 5 +-
> drivers/gpu/drm/tidss/tidss_irq.h | 2 +-
> drivers/gpu/drm/tidss/tidss_kms.c | 2 +-
> 6 files changed, 90 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index ad93acc9abd2..dbc6a5b617ca 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -93,10 +93,13 @@ const struct dispc_features dispc_k2g_feats = {
> .common_regs = tidss_k2g_common_regs,
>
> .num_vps = 1,
> + .num_output_ports = 1,
> + .oldi_supported = false,

I'd prefer "has_oldi", the style is used in other places too.

> .vp_name = { "vp1" },
> .ovr_name = { "ovr1" },
> .vpclk_name = { "vp1" },
> .vp_bus_type = { DISPC_VP_DPI },
> + .output_port_bus_type = { DISPC_VP_DPI },

It would make sense to re-arrange these a bit, so that lines related to
VPs are together, and lines related to output ports would be together.

> .vp_feat = { .color = {
> .has_ctm = true,
> @@ -168,10 +171,13 @@ const struct dispc_features dispc_am65x_feats = {
> .common_regs = tidss_am65x_common_regs,
>
> .num_vps = 2,
> + .num_output_ports = 2,
> + .oldi_supported = true,
> .vp_name = { "vp1", "vp2" },
> .ovr_name = { "ovr1", "ovr2" },
> .vpclk_name = { "vp1", "vp2" },
> .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI },
> + .output_port_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI },
>
> .vp_feat = { .color = {
> .has_ctm = true,
> @@ -257,12 +263,16 @@ const struct dispc_features dispc_j721e_feats = {
> .common_regs = tidss_j721e_common_regs,
>
> .num_vps = 4,
> + .num_output_ports = 4,
> + .oldi_supported = false,
> .vp_name = { "vp1", "vp2", "vp3", "vp4" },
> .ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" },
> .vpclk_name = { "vp1", "vp2", "vp3", "vp4" },
> /* Currently hard coded VP routing (see dispc_initial_config()) */
> .vp_bus_type = { DISPC_VP_INTERNAL, DISPC_VP_DPI,
> DISPC_VP_INTERNAL, DISPC_VP_DPI, },
> + .output_port_bus_type = { DISPC_VP_INTERNAL, DISPC_VP_DPI,
> + DISPC_VP_INTERNAL, DISPC_VP_DPI, },
> .vp_feat = { .color = {
> .has_ctm = true,
> .gamma_size = 1024,
> @@ -275,6 +285,59 @@ const struct dispc_features dispc_j721e_feats = {
> .vid_order = { 1, 3, 0, 2 },
> };
>
> +const struct dispc_features dispc_am625_feats = {
> + .max_pclk_khz = {
> + [DISPC_VP_DPI] = 165000,
> + [DISPC_VP_OLDI] = 165000,
> + },
> +
> + .scaling = {
> + .in_width_max_5tap_rgb = 1280,
> + .in_width_max_3tap_rgb = 2560,
> + .in_width_max_5tap_yuv = 2560,
> + .in_width_max_3tap_yuv = 4096,
> + .upscale_limit = 16,
> + .downscale_limit_5tap = 4,
> + .downscale_limit_3tap = 2,
> + /*
> + * The max supported pixel inc value is 255. The value
> + * of pixel inc is calculated like this: 1+(xinc-1)*bpp.
> + * The maximum bpp of all formats supported by the HW
> + * is 8. So the maximum supported xinc value is 32,
> + * because 1+(32-1)*8 < 255 < 1+(33-1)*4.
> + */
> + .xinc_max = 32,
> + },
> +
> + .subrev = DISPC_AM625,
> +
> + .common = "common",
> + .common_regs = tidss_am65x_common_regs,
> +
> + .num_vps = 2,
> + /* note: the 3rd port is not representative of a 3rd pipeline */
> + .num_output_ports = 3,
> + .oldi_supported = true,
> + .vp_name = { "vp1", "vp2" },
> + .ovr_name = { "ovr1", "ovr2" },
> + .vpclk_name = { "vp1", "vp2" },
> + .vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI },
> + .output_port_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI, DISPC_VP_OLDI },

This is a bit confusing: DISPC_VP_* defines are used for both
vp_bus_type (which makes sense), but it's also used for
output_port_bus_type.

The code uses both vp_bus_type and output_port_bus_type, but my initial
reaction was that we should only have one of these. Perhaps the
output_port_bus_type, as the VPs should be identical. The differences
are after the VP.

However, I'm not sure if that's an easy change. But maybe instead of
output_port_bus_type we could have output_port_source, which lists, for
each output port, the VP it uses as a source. Here it would be { 0, 1, 0
}. With that, the code can get the output's VP bus_type.

> + .vp_feat = { .color = {
> + .has_ctm = true,
> + .gamma_size = 256,
> + .gamma_type = TIDSS_GAMMA_8BIT,
> + },
> + },
> +
> + .num_planes = 2,
> + /* note: vid is plane_id 0 and vidl1 is plane_id 1 */
> + .vid_name = { "vid", "vidl1" },
> + .vid_lite = { false, true, },
> + .vid_order = { 1, 0 },
> +};
> +
> static const u16 *dispc_common_regmap;
>
> struct dss_vp_data {
> @@ -287,12 +350,12 @@ struct dispc_device {
>
> void __iomem *base_common;
> void __iomem *base_vid[TIDSS_MAX_PLANES];
> - void __iomem *base_ovr[TIDSS_MAX_PORTS];
> - void __iomem *base_vp[TIDSS_MAX_PORTS];
> + void __iomem *base_ovr[TIDSS_MAX_VPS];
> + void __iomem *base_vp[TIDSS_MAX_VPS];
>
> struct regmap *oldi_io_ctrl;
>
> - struct clk *vp_clk[TIDSS_MAX_PORTS];
> + struct clk *vp_clk[TIDSS_MAX_VPS];
>
> const struct dispc_features *feat;
>
> @@ -300,7 +363,7 @@ struct dispc_device {
>
> bool is_enabled;
>
> - struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
> + struct dss_vp_data vp_data[TIDSS_MAX_VPS];
>
> u32 *fourccs;
> u32 num_fourccs;
> @@ -778,6 +841,7 @@ dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc)
> return dispc_k2g_read_and_clear_irqstatus(dispc);
> case DISPC_AM65X:
> case DISPC_J721E:
> + case DISPC_AM625:

In switch cases like these, I think it makes sense to group the cases.
Here the AM6 cases should be together, and I'd put the AM625 on top so
that the cases are sorted.

> return dispc_k3_read_and_clear_irqstatus(dispc);
> default:
> WARN_ON(1);
> @@ -793,6 +857,7 @@ void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
> break;
> case DISPC_AM65X:
> case DISPC_J721E:
> + case DISPC_AM625:
> dispc_k3_set_irqenable(dispc, mask);
> break;
> default:
> @@ -1116,7 +1181,7 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
> const struct drm_display_mode *mode)
> {
> u32 hsw, hfp, hbp, vsw, vfp, vbp;
> - enum dispc_vp_bus_type bus_type;
> + enum dispc_bus_type bus_type;
> int max_pclk;
>
> bus_type = dispc->feat->vp_bus_type[hw_videoport];
> @@ -1282,6 +1347,7 @@ void dispc_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane,
> x, y, layer);
> break;
> case DISPC_AM65X:
> + case DISPC_AM625:
> dispc_am65x_ovr_set_plane(dispc, hw_plane, hw_videoport,
> x, y, layer);
> break;
> @@ -2205,6 +2271,7 @@ static void dispc_plane_init(struct dispc_device *dispc)
> break;
> case DISPC_AM65X:
> case DISPC_J721E:
> + case DISPC_AM625:
> dispc_k3_plane_init(dispc);
> break;
> default:
> @@ -2310,6 +2377,7 @@ static void dispc_vp_write_gamma_table(struct dispc_device *dispc,
> dispc_k2g_vp_write_gamma_table(dispc, hw_videoport);
> break;
> case DISPC_AM65X:
> + case DISPC_AM625:
> dispc_am65x_vp_write_gamma_table(dispc, hw_videoport);
> break;
> case DISPC_J721E:
> @@ -2583,7 +2651,7 @@ int dispc_runtime_resume(struct dispc_device *dispc)
> REG_GET(dispc, DSS_SYSSTATUS, 2, 2),
> REG_GET(dispc, DSS_SYSSTATUS, 3, 3));
>
> - if (dispc->feat->subrev == DISPC_AM65X)
> + if (dispc->feat->oldi_supported)
> dev_dbg(dispc->dev, "OLDI RESETDONE %d,%d,%d\n",
> REG_GET(dispc, DSS_SYSSTATUS, 5, 5),
> REG_GET(dispc, DSS_SYSSTATUS, 6, 6),
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
> index e49432f0abf5..51db500590ee 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
> @@ -50,7 +50,7 @@ struct dispc_errata {
> bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */
> };
>
> -enum dispc_vp_bus_type {
> +enum dispc_bus_type {
> DISPC_VP_DPI, /* DPI output */
> DISPC_VP_OLDI, /* OLDI (LVDS) output */
> DISPC_VP_INTERNAL, /* SoC internal routing */

If you rename the enum to dispc_bus_type, the enum value name doesn't
match anymore as it still contains VP. If you go with the suggestion of
not having output_port_bus_type at all, then you can keep this as
dispc_vp_bus_type.

But if we need types for both VPs and outputs, I think it would be
better to have separate enums for them.

> @@ -61,6 +61,7 @@ enum dispc_dss_subrevision {
> DISPC_K2G,
> DISPC_AM65X,
> DISPC_J721E,
> + DISPC_AM625,
> };
>
> struct dispc_features {
> @@ -74,10 +75,13 @@ struct dispc_features {
> const char *common;
> const u16 *common_regs;
> u32 num_vps;
> - const char *vp_name[TIDSS_MAX_PORTS]; /* Should match dt reg names */
> - const char *ovr_name[TIDSS_MAX_PORTS]; /* Should match dt reg names */
> - const char *vpclk_name[TIDSS_MAX_PORTS]; /* Should match dt clk names */
> - const enum dispc_vp_bus_type vp_bus_type[TIDSS_MAX_PORTS];
> + u32 num_output_ports;
> + bool oldi_supported;
> + const char *vp_name[TIDSS_MAX_VPS]; /* Should match dt reg names */
> + const char *ovr_name[TIDSS_MAX_VPS]; /* Should match dt reg names */
> + const char *vpclk_name[TIDSS_MAX_VPS]; /* Should match dt clk names */
> + const enum dispc_bus_type vp_bus_type[TIDSS_MAX_VPS];
> + const enum dispc_bus_type output_port_bus_type[TIDSS_MAX_OUTPUT_PORTS];
> struct tidss_vp_feat vp_feat;
> u32 num_planes;
> const char *vid_name[TIDSS_MAX_PLANES]; /* Should match dt reg names */
> @@ -88,6 +92,7 @@ struct dispc_features {
> extern const struct dispc_features dispc_k2g_feats;
> extern const struct dispc_features dispc_am65x_feats;
> extern const struct dispc_features dispc_j721e_feats;
> +extern const struct dispc_features dispc_am625_feats;
>
> void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask);
> dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc);
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index 15cd9b91b7e2..c5e119828823 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -235,6 +235,7 @@ static const struct of_device_id tidss_of_table[] = {
> { .compatible = "ti,k2g-dss", .data = &dispc_k2g_feats, },
> { .compatible = "ti,am65x-dss", .data = &dispc_am65x_feats, },
> { .compatible = "ti,j721e-dss", .data = &dispc_j721e_feats, },
> + { .compatible = "ti,am625-dss", .data = &dispc_am625_feats, },
> { }
> };
>
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
> index d7f27b0b0315..0ce7ee5ccd5b 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.h
> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
> @@ -9,8 +9,9 @@
>
> #include <linux/spinlock.h>
>
> -#define TIDSS_MAX_PORTS 4
> +#define TIDSS_MAX_VPS 4
> #define TIDSS_MAX_PLANES 4
> +#define TIDSS_MAX_OUTPUT_PORTS 4
>
> typedef u32 dispc_irq_t;
>
> @@ -22,7 +23,7 @@ struct tidss_device {
> struct dispc_device *dispc;
>
> unsigned int num_crtcs;
> - struct drm_crtc *crtcs[TIDSS_MAX_PORTS];
> + struct drm_crtc *crtcs[TIDSS_MAX_VPS];
>
> unsigned int num_planes;
> struct drm_plane *planes[TIDSS_MAX_PLANES];
> diff --git a/drivers/gpu/drm/tidss/tidss_irq.h b/drivers/gpu/drm/tidss/tidss_irq.h
> index b512614d5863..a753f5e3ce15 100644
> --- a/drivers/gpu/drm/tidss/tidss_irq.h
> +++ b/drivers/gpu/drm/tidss/tidss_irq.h
> @@ -35,7 +35,7 @@
>
> #define DSS_IRQ_VP_BIT_N(ch, bit) (4 + 4 * (ch) + (bit))
> #define DSS_IRQ_PLANE_BIT_N(plane, bit) \
> - (DSS_IRQ_VP_BIT_N(TIDSS_MAX_PORTS, 0) + 1 * (plane) + (bit))
> + (DSS_IRQ_VP_BIT_N(TIDSS_MAX_VPS, 0) + 1 * (plane) + (bit))
>
> #define DSS_IRQ_VP_BIT(ch, bit) BIT(DSS_IRQ_VP_BIT_N((ch), (bit)))
> #define DSS_IRQ_PLANE_BIT(plane, bit) \
> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> index afb2879980c6..fc9c4eefd31d 100644
> --- a/drivers/gpu/drm/tidss/tidss_kms.c
> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
> @@ -123,7 +123,7 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
> u32 max_vps = feat->num_vps;
> u32 max_planes = feat->num_planes;
>
> - struct pipe pipes[TIDSS_MAX_PORTS];
> + struct pipe pipes[TIDSS_MAX_VPS];
> u32 num_pipes = 0;
> u32 crtc_mask;
>

Tomi

2023-01-24 06:40:48

by Aradhya Bhatia

[permalink] [raw]
Subject: Re: [PATCH v6 3/5] drm/tidss: Add support to configure OLDI mode for am625-dss.

Hi Tomi,

Thanks for reviewing the patch series. I have implemented the most of
your suggestions, but for the others, I needed to clarify things. I have
made some comments there.

On 20-Dec-22 18:22, Tomi Valkeinen wrote:
> Hi,
>
> On 19/11/2022 19:30, Aradhya Bhatia wrote:
>> The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal.
>> These can be configured to support the following modes:
>>
>> 1. OLDI_SINGLE_LINK_SINGLE_MODE
>> Single Output over OLDI 0.
>> +------+        +---------+      +-------+
>> |      |        |         |      |       |
>> | CRTC +------->+ ENCODER +----->| PANEL |
>> |      |        |         |      |       |
>> +------+        +---------+      +-------+
>>
>> 2. OLDI_SINGLE_LINK_CLONE_MODE
>> Duplicate Output over OLDI 0 and 1.
>> +------+        +---------+      +-------+
>> |      |        |         |      |       |
>> | CRTC +---+--->| ENCODER +----->| PANEL |
>> |      |   |    |         |      |       |
>> +------+   |    +---------+      +-------+
>>             |
>>             |    +---------+      +-------+
>>             |    |         |      |       |
>>             +--->| ENCODER +----->| PANEL |
>>                  |         |      |       |
>>                  +---------+      +-------+
>>
>> 3. OLDI_DUAL_LINK_MODE
>> Combined Output over OLDI 0 and 1.
>> +------+        +---------+      +-------+
>> |      |        |         +----->|       |
>> | CRTC +------->+ ENCODER |      | PANEL |
>> |      |        |         +----->|       |
>> +------+        +---------+      +-------+
>>
>> Following the above pathways for different modes, 2 encoder/panel-bridge
>> pipes get created for clone mode, and 1 pipe in cases of single link and
>> dual link mode.
>>
>> Add support for confguring the OLDI modes using OF and LVDS DRM helper
>> functions.
>>
>> Signed-off-by: Aradhya Bhatia <[email protected]>
>> ---
>>   drivers/gpu/drm/tidss/tidss_dispc.c   |  12 ++
>>   drivers/gpu/drm/tidss/tidss_dispc.h   |   9 ++
>>   drivers/gpu/drm/tidss/tidss_drv.h     |   3 +
>>   drivers/gpu/drm/tidss/tidss_encoder.c |   4 +-
>>   drivers/gpu/drm/tidss/tidss_encoder.h |   3 +-
>>   drivers/gpu/drm/tidss/tidss_kms.c     | 188 +++++++++++++++++++++++---
>>   6 files changed, 198 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c
>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index dbc6a5b617ca..472226a83251 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -365,6 +365,8 @@ struct dispc_device {
>>       struct dss_vp_data vp_data[TIDSS_MAX_VPS];
>> +    enum dispc_oldi_modes oldi_mode;
>> +
>>       u32 *fourccs;
>>       u32 num_fourccs;
>> @@ -1967,6 +1969,16 @@ const u32 *dispc_plane_formats(struct
>> dispc_device *dispc, unsigned int *len)
>>       return dispc->fourccs;
>>   }
>> +int dispc_set_oldi_mode(struct dispc_device *dispc,
>> +            enum dispc_oldi_modes oldi_mode)
>> +{
>> +    WARN_ON(!dispc);
>
> This feels unnecessary. Is there even a theoretical case where we could
> get dispc == NULL?
>
>> +
>> +    dispc->oldi_mode = oldi_mode;
>> +
>> +    return 0;
>
> This function could as well be void function. >
>> +}
>> +
>>   static s32 pixinc(int pixels, u8 ps)
>>   {
>>       if (pixels == 1)
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h
>> b/drivers/gpu/drm/tidss/tidss_dispc.h
>> index 51db500590ee..e76a7599b544 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
>> @@ -64,6 +64,14 @@ enum dispc_dss_subrevision {
>>       DISPC_AM625,
>>   };
>> +enum dispc_oldi_modes {
>> +    OLDI_MODE_OFF,            /* OLDI turned off / tied off in IP. */
>> +    OLDI_SINGLE_LINK_SINGLE_MODE,    /* Single Output over OLDI 0. */
>> +    OLDI_SINGLE_LINK_CLONE_MODE,    /* Duplicate Output over OLDI 0 and 1. */
>> +    OLDI_DUAL_LINK_MODE,        /* Combined Output over OLDI 0 and 1. */
>> +    OLDI_MODE_UNSUPPORTED,        /* Unsupported OLDI Mode */
>> +};
>
> What is the difference with MODE_OFF and MODE_UNSUPPORTED? Is
> MODE_UNSUPPORTED for cases where, e.g., the DT setup is wrong and the
> driver should return an error? The code doesn't quite do that, it prints
> an error but then continues.

Yes, OLDI_MODE_UNSUPPORTED is for the cases where DT setup is wrong.
I have not exited in such a cases with an error, because then the driver
will never have a chance to setup the 2nd pipeline (DPI) even if all the
DT requirements are met.

>
>>   struct dispc_features {
>>       int min_pclk_khz;
>>       int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
>> @@ -133,6 +141,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
>>                 u32 hw_videoport);
>>   int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable);
>>   const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len);
>> +int dispc_set_oldi_mode(struct dispc_device *dispc, enum dispc_oldi_modes oldi_mode);
>>   int dispc_init(struct tidss_device *tidss);
>>   void dispc_remove(struct tidss_device *tidss);
>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h
>> b/drivers/gpu/drm/tidss/tidss_drv.h
>> index 0ce7ee5ccd5b..58892f065c16 100644
>> --- a/drivers/gpu/drm/tidss/tidss_drv.h
>> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
>> @@ -13,6 +13,9 @@
>>   #define TIDSS_MAX_PLANES 4
>>   #define TIDSS_MAX_OUTPUT_PORTS 4
>> +/* For AM625-DSS with 2 OLDI TXes */
>> +#define TIDSS_MAX_BRIDGES_PER_PIPE    2
>> +
>>   typedef u32 dispc_irq_t;
>>   struct tidss_device {
>> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c
>> b/drivers/gpu/drm/tidss/tidss_encoder.c
>> index e278a9c89476..141383ec4045 100644
>> --- a/drivers/gpu/drm/tidss/tidss_encoder.c
>> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
>> @@ -70,7 +70,8 @@ static const struct drm_encoder_funcs encoder_funcs = {
>>   };
>>   struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>> -                     u32 encoder_type, u32 possible_crtcs)
>> +                     u32 encoder_type, u32 possible_crtcs,
>> +                     u32 possible_clones)
>>   {
>>       struct drm_encoder *enc;
>>       int ret;
>> @@ -80,6 +81,7 @@ struct drm_encoder *tidss_encoder_create(struct
>> tidss_device *tidss,
>>           return ERR_PTR(-ENOMEM);
>>       enc->possible_crtcs = possible_crtcs;
>> +    enc->possible_clones = possible_clones;
>>       ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs,
>>                      encoder_type, NULL);
>> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h
>> b/drivers/gpu/drm/tidss/tidss_encoder.h
>> index ace877c0e0fd..01c62ba3ef16 100644
>> --- a/drivers/gpu/drm/tidss/tidss_encoder.h
>> +++ b/drivers/gpu/drm/tidss/tidss_encoder.h
>> @@ -12,6 +12,7 @@
>>   struct tidss_device;
>>   struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>> -                     u32 encoder_type, u32 possible_crtcs);
>> +                     u32 encoder_type, u32 possible_crtcs,
>> +                     u32 possible_clones);
>>   #endif
>> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c
>> b/drivers/gpu/drm/tidss/tidss_kms.c
>> index fc9c4eefd31d..8ae321f02197 100644
>> --- a/drivers/gpu/drm/tidss/tidss_kms.c
>> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
>> @@ -106,30 +106,115 @@ static const struct drm_mode_config_funcs
>> mode_config_funcs = {
>>       .atomic_commit = drm_atomic_helper_commit,
>>   };
>> +static enum dispc_oldi_modes tidss_get_oldi_mode(struct device_node
>> *oldi0_port,
>> +                         struct device_node *oldi1_port)
>> +{
>> +    int pixel_order;
>> +
>> +    if (!(oldi0_port || oldi1_port)) {
>> +        /* Keep OLDI TXes off if neither OLDI port is present. */
>> +        return OLDI_MODE_OFF;
>> +    } else if (oldi0_port && !oldi1_port) {
>> +        /*
>> +         * OLDI0 port found, but not OLDI1 port. Setting single
>> +         * link, single mode output.
>> +         */
>> +        return OLDI_SINGLE_LINK_SINGLE_MODE;
>> +    } else if (!oldi0_port && oldi1_port) {
>> +        /*
>> +         * The 2nd OLDI TX cannot be operated alone. This use case is
>> +         * not supported in the HW. Since the pins for OLDIs 0 and 1 are
>> +         * separate, one could theoretically set a clone mode over OLDIs
>> +         * 0 and 1 and just simply not use the OLDI 0. This is a hacky
>> +         * way to enable only OLDI TX 1 and hence is not officially
>> +         * supported.
>> +         */
>> +        return OLDI_MODE_UNSUPPORTED;
>
> If OLDI_MODE_UNSUPPORTED is supposed to result in an error, maybe you
> could print the error here (and possibly in the default case below), and
> then, in the caller, just return with an error code.
>
>> +    }
>> +
>> +    /*
>> +     * OLDI Ports found for both the OLDI TXes. The DSS is to be configured
>> +     * in either Dual Link or Clone Mode.
>> +     */
>> +    pixel_order = drm_of_lvds_get_dual_link_pixel_order(oldi0_port,
>> +                                oldi1_port);
>> +    switch (pixel_order) {
>> +    case -EINVAL:
>> +        /*
>> +         * The dual link properties were not found in at least one of
>> +         * the sink nodes. Since 2 OLDI ports are present in the DT, it
>> +         * can be safely assumed that the required configuration is
>> +         * Clone Mode.
>> +         */
>> +        return OLDI_SINGLE_LINK_CLONE_MODE;
>> +
>> +    case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
>> +        /*
>> +         * Note that the OLDI TX 0 transmits the odd set of pixels while
>> +         * the OLDI TX 1 transmits the even set. This is a fixed
>> +         * configuration in the IP and an cannot be change vis SW. These
>> +         * properties have been used to merely identify if a Dual Link
>> +         * configuration is required. Swapping this property in the panel
>> +         * port DT nodes will not make any difference.
>> +         */
>> +        pr_warn("EVEN-ODD config for dual-link sinks is not supported in HW. Switching to ODD-EVEN.\n");
>
> Please use dev_warn() instead, so that it will be clear where the print
> comes from.
>
> In any case, isn't this an error? Do you really want to accept the wrong
> pixel order?
>
>> +        fallthrough;
>> +
>> +    case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
>> +        return OLDI_DUAL_LINK_MODE;
>> +
>> +    default:
>> +        return OLDI_MODE_OFF;
>
> When do we get here? Shouldn't this be OLDI_MODE_UNSUPPORTED?
>
>> +    }
>> +}
>> +
>>   static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>>   {
>>       struct device *dev = tidss->dev;
>>       unsigned int fourccs_len;
>>       const u32 *fourccs = dispc_plane_formats(tidss->dispc,
>> &fourccs_len);
>> -    unsigned int i;
>> +    unsigned int i, j;
>>       struct pipe {
>>           u32 hw_videoport;
>> -        struct drm_bridge *bridge;
>> +        struct drm_bridge *bridge[TIDSS_MAX_BRIDGES_PER_PIPE];
>>           u32 enc_type;
>> +        u32 num_bridges;
>>       };
>>       const struct dispc_features *feat = tidss->feat;
>> -    u32 max_vps = feat->num_vps;
>> +    u32 output_ports = feat->num_output_ports;
>>       u32 max_planes = feat->num_planes;
>> -    struct pipe pipes[TIDSS_MAX_VPS];
>> +    struct pipe pipes[TIDSS_MAX_VPS] = {0};
>> +
>>       u32 num_pipes = 0;
>>       u32 crtc_mask;
>> +    u32 portnum_oldi0 = 0, portnum_oldi1 = 2;
>
> These two are bit of hacks. First, they should be const, or maybe
> defines. If they're const, they can be inside the block below.
>
> And they're very much tied to the HW in question, so just having
> hardcoded values here inside a function without any mention of the
> situation is not good.
>
> Doing this in a generic and future proof manner is... challenging. So I
> think using the hardcoded port numbers is fine. But they are only ok for
> the two AM6xx SoCs we have now so the 'oldi_supported' is not very good
> fit. In fact, it might be good to drop 'oldi_supported' altogether, and
> just check for the SoC versions instead, as (with a quick glance), all
> the 'oldi_supported' checks are really SoC specific.
>
I will be make these portnum variables const as you suggested and
moving these as well as get-node and put-node function calls to the
get_oldi_mode function to keep things clear.

However, I believe the 'oldi_supported' variable should still be kept
(after renaming it to has_oldi as per your suggestion in the previous
patch) because having this variable will help distinguish from the cases
where an SoC *can* support OLDI output but its output by-passes the OLDI
TXes and a DPI output is expected.

> This also again brings up a thing that rubs me the wrong way: the new
> OLDI port is port 2. I really think that on AM62x, the two OLDI ports
> should be 0 and 1, and the DPI should be 2. Would we need a new
> dt-binding doc for that, or could it still be described in the same doc?
> Would that change cause changes elsewhere in the dss driver?
>
>> +    enum dispc_oldi_modes oldi_mode = OLDI_MODE_OFF;
>> +    u32 num_oldi = 0;
>> +    u32 oldi_pipe_index = 0;
>> +    u32 num_encoders = 0;
>> +
>> +    if (feat->oldi_supported) {
>> +        struct device_node *oldi0_port, *oldi1_port;
>> +
>> +        oldi0_port = of_graph_get_port_by_id(dev->of_node,
>> +                             portnum_oldi0);
>> +        oldi1_port = of_graph_get_port_by_id(dev->of_node,
>> +                             portnum_oldi1);
>> +
>> +        oldi_mode = tidss_get_oldi_mode(oldi0_port, oldi1_port);
>> +
>> +        of_node_put(oldi0_port);
>> +        of_node_put(oldi1_port);
>> +
>> +        dispc_set_oldi_mode(tidss->dispc, oldi_mode);
>> +    }
>>       /* first find all the connected panels & bridges */
>> -    for (i = 0; i < max_vps; i++) {
>> +    for (i = 0; i < output_ports; i++) {
>>           struct drm_panel *panel;
>>           struct drm_bridge *bridge;
>>           u32 enc_type = DRM_MODE_ENCODER_NONE;
>> @@ -145,16 +230,24 @@ static int tidss_dispc_modeset_init(struct
>> tidss_device *tidss)
>>               return ret;
>>           }
>> +        if (feat->output_port_bus_type[i] == DISPC_VP_OLDI &&
>> +            oldi_mode == OLDI_MODE_UNSUPPORTED) {
>> +            dev_err(dev,
>> +                "Single Mode over OLDI 1 is not supported in HW. Keeping OLDI off.\n");
>> +            continue;
>
> Should we error out here?
>
>> +        }
>> +
>>           if (panel) {
>>               u32 conn_type;
>>               dev_dbg(dev, "Setting up panel for port %d\n", i);
>> -            switch (feat->vp_bus_type[i]) {
>> +            switch (feat->output_port_bus_type[i]) {
>>               case DISPC_VP_OLDI:
>>                   enc_type = DRM_MODE_ENCODER_LVDS;
>>                   conn_type = DRM_MODE_CONNECTOR_LVDS;
>>                   break;
>> +
>>               case DISPC_VP_DPI:
>>                   enc_type = DRM_MODE_ENCODER_DPI;
>>                   conn_type = DRM_MODE_CONNECTOR_DPI;
>> @@ -172,6 +265,17 @@ static int tidss_dispc_modeset_init(struct
>> tidss_device *tidss)
>>                   return -EINVAL;
>>               }
>> +            /*
>> +             * If the 2nd OLDI port is discovered connected to a panel
>> +             * which is not to be connected in the Clone Mode then a
>> +             * bridge is not required because the detected port is the
>> +             * 2nd port for the previously connected panel.
>> +             */
>> +            if (feat->output_port_bus_type[i] == DISPC_VP_OLDI &&
>> +                oldi_mode != OLDI_SINGLE_LINK_CLONE_MODE &&
>
> Hmm, shouldn't this be oldi_mode == OLDI_DUAL_LINK_MODE? Or rather,
Yes. I will make the change...

> shouldn't we test here if this is the second oldi display, and if so
> which oldi-mode are we using. If dual-link, break. If clone, continue.
> Otherwise, error.
but, if I implement the oldi-mode-find logic over here, that would be
limited to panels and will skip out the bridges. And the mode-find
should really be only done once.

That said, I see that this can get a little confusing, So, while keeping
the mode-find logic separate, I have organized the other OLDI specific
things separately in the next patch.

>
>> +                num_oldi)
>> +                break;
>> +
>>               bridge = devm_drm_panel_bridge_add(dev, panel);
>>               if (IS_ERR(bridge)) {
>>                   dev_err(dev,
>> @@ -181,10 +285,47 @@ static int tidss_dispc_modeset_init(struct
>> tidss_device *tidss)
>>               }
>>           }
>> -        pipes[num_pipes].hw_videoport = i;
>> -        pipes[num_pipes].bridge = bridge;
>> -        pipes[num_pipes].enc_type = enc_type;
>> -        num_pipes++;
>> +        if (feat->output_port_bus_type[i] == DISPC_VP_OLDI) {
>> +            if (++num_oldi == 1) {
>> +                /* Setting up pipe parameters when 1st OLDI port is detected */
>> +
>> +                pipes[num_pipes].hw_videoport = i;
>> +                pipes[num_pipes].enc_type = enc_type;
>> +
>> +                /*
>> +                 * Saving the pipe index in case its required for
>> +                 * 2nd OLDI Port.
>> +                 */
>> +                oldi_pipe_index = num_pipes;
>> +
>> +                /*
>> +                 * No additional pipe is required for the 2nd OLDI
>> +                 * port, because the 2nd Encoder -> Bridge connection
>> +                 * is the part of the first OLDI Port pipe.
>> +                 *
>> +                 * num_pipes will only be incremented when the first
>> +                 * OLDI port is discovered.
>> +                 */
>> +                num_pipes++;
>> +            }
>> +
>> +            /*
>> +             * Bridge is required to be added only if the detected
>> +             * port is the first OLDI port or a subsequent port in
>> +             * Clone Mode.
>> +             */
>> +            if (oldi_mode == OLDI_SINGLE_LINK_CLONE_MODE ||
>> +                num_oldi == 1) {
>> +                pipes[oldi_pipe_index].bridge[num_oldi - 1] = bridge;
>> +                pipes[oldi_pipe_index].num_bridges++;
>> +            }
>> +        } else {
>> +            pipes[num_pipes].hw_videoport = i;
>> +            pipes[num_pipes].bridge[0] = bridge;
>> +            pipes[num_pipes].num_bridges++;
>> +            pipes[num_pipes].enc_type = enc_type;
>> +            num_pipes++;
>> +        }
>>       }
>>       /* all planes can be on any crtc */
>> @@ -196,6 +337,7 @@ static int tidss_dispc_modeset_init(struct
>> tidss_device *tidss)
>>           struct tidss_plane *tplane;
>>           struct tidss_crtc *tcrtc;
>>           struct drm_encoder *enc;
>> +        u32 possible_clones = 0;
>>           u32 hw_plane_id = feat->vid_order[tidss->num_planes];
>>           int ret;
>> @@ -218,16 +360,24 @@ static int tidss_dispc_modeset_init(struct
>> tidss_device *tidss)
>>           tidss->crtcs[tidss->num_crtcs++] = &tcrtc->crtc;
>> -        enc = tidss_encoder_create(tidss, pipes[i].enc_type,
>> -                       1 << tcrtc->crtc.index);
>> -        if (IS_ERR(enc)) {
>> -            dev_err(tidss->dev, "encoder create failed\n");
>> -            return PTR_ERR(enc);
>> -        }
>> +        for (j = 0; j < pipes[i].num_bridges; j++) {
>> +            if (pipes[i].num_bridges > 1)
>> +                possible_clones = (((1 << pipes[i].num_bridges) - 1)
>> +                          << num_encoders);
>
> I think the above possible_clones assignment can be outside the for loop. >
>> +
>> +            enc = tidss_encoder_create(tidss, pipes[i].enc_type,
>> +                           1 << tcrtc->crtc.index,
>> +                           possible_clones);
>> +            if (IS_ERR(enc)) {
>> +                dev_err(tidss->dev, "encoder create failed\n");
>> +                return PTR_ERR(enc);
>> +            }
>> -        ret = drm_bridge_attach(enc, pipes[i].bridge, NULL, 0);
>> -        if (ret)
>> -            return ret;
>> +            ret = drm_bridge_attach(enc, pipes[i].bridge[j], NULL, 0);
>> +            if (ret)
>> +                return ret;
>> +        }
>> +        num_encoders += pipes[i].num_bridges;
>>       }
>>       /* create overlay planes of the leftover planes */

Regards
Aradhya