This patch series adds the required support for enabling OLDI display on
boards having SoCs with am625 compatible Display SubSystem.
The 2 OLDI TXes on am625-dss allow a 3 different types of panel
connections with the board.
1. Single Link / Single Mode on OLDI TX 0 OR 1.
2. Single Link / Duplicate Mode on OLDI TX 0 and 1.
3. Dual Link / Single Mode on OLDI TX 0 and 1.
The 3rd combination allows the DSS to be connected to LVDS panels of
resolutions upto 2K.
This patch series further adds support for boards, with HDMI bridges
supporting only 24bit RGB color space, to be able to explicitly output
16bit RGB data when only 16 DPI output pins have been connected from SoC
to bridge.
Note:
These patches require the new compatible "ti,am625-dss" and its support
in tidss driver. Those patches have been previously posted and can be
found on the following link:
https://patchwork.kernel.org/project/dri-devel/list/?series=654214&archive=both
Aradhya Bhatia (8):
dt-bindings: display: ti,am65x-dss: Add port properties for DSS
dt-bindings: display: ti,am65x-dss: Add IO CTRL property for AM625
OLDI
drm/tidss: Add support for DSS port properties
drm/tidss: Add support for Dual Link LVDS Bus Format
drm/tidss: dt property to force 16bit VP output to a 24bit bridge
drm/tidss: Add IO CTRL and Power support for OLDI TX in am625
drm/tidss: Fix clock request value for OLDI videoports
drm/tidss: Enable Dual and Duplicate Modes for OLDI
.../bindings/display/ti/ti,am65x-dss.yaml | 46 +++-
drivers/gpu/drm/tidss/tidss_dispc.c | 220 +++++++++++++++---
drivers/gpu/drm/tidss/tidss_dispc.h | 8 +
drivers/gpu/drm/tidss/tidss_dispc_regs.h | 6 +
4 files changed, 246 insertions(+), 34 deletions(-)
--
2.37.0
Add am625-io-ctrl dt property to provide access to the control MMR
registers for the OLDI TXes.
These registers are used to control the power input to the OLDI TXes as
well as to configure them in the Loopback test mode.
The MMR IO controller device has been updated since the AM65x SoC and
hence a newer property is needed to describe the one in AM625 SoC.
Signed-off-by: Aradhya Bhatia <[email protected]>
---
.../bindings/display/ti/ti,am65x-dss.yaml | 21 +++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index 11d9b3821409..672765ad1f30 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -118,12 +118,33 @@ properties:
and OLDI_CLK_IO_CTRL registers. This property is needed for OLDI
interface to work.
+ ti,am625-oldi-io-ctrl:
+ $ref: "/schemas/types.yaml#/definitions/phandle"
+ description:
+ phandle to syscon device node mapping OLDI IO_CTRL registers, for
+ AM625 SoC. The mapped range should point to OLDI0_DAT0_IO_CTRL,
+ and map the registers up till OLDI_LB_CTRL. This property allows
+ the driver to control the power delivery to the OLDI TXes and
+ their loopback control as well.
+
max-memory-bandwidth:
$ref: /schemas/types.yaml#/definitions/uint32
description:
Input memory (from main memory to dispc) bandwidth limit in
bytes per second
+if:
+ properties:
+ compatible:
+ contains:
+ const: ti,am65x-dss
+then:
+ properties:
+ ti,am625-oldi-io-ctrl: false
+else:
+ properties:
+ ti,am65x-oldi-io-ctrl: false
+
required:
- compatible
- reg
--
2.37.0
The 2 OLDI TXes in the AM625 SoC can be synced together to output a 2K
resolution video.
Add support in the driver for the discovery of such a dual mode
connection on the OLDI video port, using the values of "ti,oldi-mode"
property.
Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/tidss/tidss_dispc.c | 39 +++++++++++++++++++++--------
1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index add725fa682b..fb1fdecfc83a 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -853,25 +853,36 @@ void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
}
}
-enum dispc_oldi_mode_reg_val { SPWG_18 = 0, JEIDA_24 = 1, SPWG_24 = 2 };
+enum dispc_oldi_mode_reg_val {
+ SPWG_18 = 0,
+ JEIDA_24 = 1,
+ SPWG_24 = 2,
+ DL_SPWG_18 = 4,
+ DL_JEIDA_24 = 5,
+ DL_SPWG_24 = 6,
+};
struct dispc_bus_format {
u32 bus_fmt;
u32 data_width;
bool is_oldi_fmt;
+ bool is_dual_link;
enum dispc_oldi_mode_reg_val oldi_mode_reg_val;
};
static const struct dispc_bus_format dispc_bus_formats[] = {
- { MEDIA_BUS_FMT_RGB444_1X12, 12, false, 0 },
- { MEDIA_BUS_FMT_RGB565_1X16, 16, false, 0 },
- { MEDIA_BUS_FMT_RGB666_1X18, 18, false, 0 },
- { MEDIA_BUS_FMT_RGB888_1X24, 24, false, 0 },
- { MEDIA_BUS_FMT_RGB101010_1X30, 30, false, 0 },
- { MEDIA_BUS_FMT_RGB121212_1X36, 36, false, 0 },
- { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, SPWG_18 },
- { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, SPWG_24 },
- { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, JEIDA_24 },
+ { MEDIA_BUS_FMT_RGB444_1X12, 12, false, false, 0 },
+ { MEDIA_BUS_FMT_RGB565_1X16, 16, false, false, 0 },
+ { MEDIA_BUS_FMT_RGB666_1X18, 18, false, false, 0 },
+ { MEDIA_BUS_FMT_RGB888_1X24, 24, false, false, 0 },
+ { MEDIA_BUS_FMT_RGB101010_1X30, 30, false, false, 0 },
+ { MEDIA_BUS_FMT_RGB121212_1X36, 36, false, false, 0 },
+ { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, false, SPWG_18 },
+ { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, false, SPWG_24 },
+ { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, false, JEIDA_24 },
+ { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, true, DL_SPWG_18 },
+ { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, true, DL_SPWG_24 },
+ { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, true, DL_JEIDA_24 },
};
static const
@@ -880,9 +891,15 @@ struct dispc_bus_format *dispc_vp_find_bus_fmt(struct dispc_device *dispc,
u32 bus_fmt, u32 bus_flags)
{
unsigned int i;
+ bool is_dual_link = false;
+
+ if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI &&
+ dispc->oldi_mode == OLDI_DUAL_LINK)
+ is_dual_link = true;
for (i = 0; i < ARRAY_SIZE(dispc_bus_formats); ++i) {
- if (dispc_bus_formats[i].bus_fmt == bus_fmt)
+ if (dispc_bus_formats[i].bus_fmt == bus_fmt &&
+ dispc_bus_formats[i].is_dual_link == is_dual_link)
return &dispc_bus_formats[i];
}
--
2.37.0
The AM625 DSS peripheral supports 2 OLDI TXes which can work to enable 2
duplicated displays of smaller resolutions or enable a single Dual-Link
display with a higher resolution (1920x1200).
Configure the necessary register to enable the different modes.
Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/tidss/tidss_dispc.c | 44 +++++++++++++++++++++++++++--
1 file changed, 41 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 0b9689453ee8..28cb61259471 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -1021,8 +1021,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
+ * set statically to 0.
*/
if (fmt->data_width == 24)
@@ -1039,7 +1039,45 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
oldi_cfg |= BIT(0); /* ENABLE */
- dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+ /*
+ * As per all the current implementations of DSS, the OLDI TXes are present only on
+ * hw_videoport = 0 (OLDI TX 0). However, the config register for 2nd OLDI TX (OLDI TX 1)
+ * is present in the address space of hw_videoport = 1. Hence, using "hw_videoport + 1" to
+ * configure OLDI TX 1.
+ */
+
+ switch (dispc->oldi_mode) {
+ case OLDI_MODE_OFF:
+ oldi_cfg &= ~BIT(0); /* DISABLE */
+ dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+ dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+ break;
+
+ case OLDI_SINGLE_LINK_SINGLE_MODE_0:
+ dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+ break;
+
+ case OLDI_SINGLE_LINK_SINGLE_MODE_1:
+ dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+ break;
+
+ case OLDI_SINGLE_LINK_DUPLICATE_MODE:
+ oldi_cfg |= BIT(5); /* DUPLICATE MODE */
+ dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+ dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+ break;
+
+ case OLDI_DUAL_LINK:
+ oldi_cfg |= BIT(11); /* DUALMODESYNC */
+ dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+ dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+ break;
+
+ default:
+ dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
+ __func__);
+ return;
+ }
while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
count < 10000)
--
2.37.0
The OLDI TX(es) require a serial clock which is 7 times the pixel clock
of the display panel. When the OLDI is enabled in DSS, the pixel clock
input of the corresponding videoport gets a divided-by 7 value of the
requested clock.
For the am625-dss, the requested clock needs to be 7 times the value.
Update the clock frequency by requesting 7 times the value.
Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/tidss/tidss_dispc.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index c4a5f808648f..0b9689453ee8 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -1326,6 +1326,16 @@ int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
int r;
unsigned long new_rate;
+ /*
+ * For AM625 OLDI video ports, the requested pixel clock needs to take into account the
+ * serial clock required for the serialization of DPI signals into LVDS signals. The
+ * incoming pixel clock on the OLDI video port gets divided by 7 whenever OLDI enable bit
+ * gets set.
+ */
+ if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI &&
+ dispc->feat->subrev == DISPC_AM625)
+ rate *= 7;
+
r = clk_set_rate(dispc->vp_clk[hw_videoport], rate);
if (r) {
dev_err(dispc->dev, "vp%d: failed to set clk rate to %lu\n",
--
2.37.0
The ctrl MMR module of the AM625 is different from the AM65X SoC. As a
result, the memory-mapped regsiters that control the OLDI TX power and
loopback have diverged in AM625 SoC.
Add support for the controller in AM625 and control OLDI.
Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/tidss/tidss_dispc.c | 70 +++++++++++++++++++-----
drivers/gpu/drm/tidss/tidss_dispc_regs.h | 6 ++
2 files changed, 62 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 94fce035c1d7..c4a5f808648f 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -934,21 +934,57 @@ 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 : OLDI_PWRDN_TX;
+
+ 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);
+
+ } else if (dispc->feat->subrev == DISPC_AM625) {
+ if (power) {
+ switch (dispc->oldi_mode) {
+ case OLDI_SINGLE_LINK_SINGLE_MODE_0:
+ /* Power down OLDI TX 1*/
+ val = OLDI1_PWRDN_TX;
+ break;
+
+ case OLDI_SINGLE_LINK_SINGLE_MODE_1:
+ /* Power down OLDI TX 0*/
+ val = OLDI0_PWRDN_TX;
+ break;
+
+ case OLDI_SINGLE_LINK_DUPLICATE_MODE:
+ case OLDI_DUAL_LINK:
+ /* No Power down */
+ val = 0;
+ break;
+
+ default:
+ /* Power down both the OLDI TXes */
+ val = OLDI0_PWRDN_TX | OLDI1_PWRDN_TX;
+ break;
+ }
+ } else {
+ /* Power down both the OLDI TXes */
+ val = OLDI0_PWRDN_TX | OLDI1_PWRDN_TX;
+ }
+
+ regmap_update_bits(dispc->oldi_io_ctrl, OLDI_PD_CTRL,
+ OLDI0_PWRDN_TX | OLDI1_PWRDN_TX, val);
+ }
}
static void dispc_set_num_datalines(struct dispc_device *dispc,
@@ -2701,9 +2737,15 @@ static int dispc_iomap_resource(struct platform_device *pdev, const char *name,
static int dispc_init_am65x_oldi_io_ctrl(struct device *dev,
struct dispc_device *dispc)
{
- dispc->oldi_io_ctrl =
- syscon_regmap_lookup_by_phandle(dev->of_node,
- "ti,am65x-oldi-io-ctrl");
+ if (dispc->feat->subrev == DISPC_AM65X)
+ dispc->oldi_io_ctrl =
+ syscon_regmap_lookup_by_phandle(dev->of_node,
+ "ti,am65x-oldi-io-ctrl");
+ else
+ dispc->oldi_io_ctrl =
+ syscon_regmap_lookup_by_phandle(dev->of_node,
+ "ti,am625-oldi-io-ctrl");
+
if (PTR_ERR(dispc->oldi_io_ctrl) == -ENODEV) {
dispc->oldi_io_ctrl = NULL;
} else if (IS_ERR(dispc->oldi_io_ctrl)) {
diff --git a/drivers/gpu/drm/tidss/tidss_dispc_regs.h b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
index 13feedfe5d6d..510bee70b3b8 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
@@ -238,6 +238,12 @@ enum dispc_common_regs {
#define OLDI_DAT3_IO_CTRL 0x0C
#define OLDI_CLK_IO_CTRL 0x10
+/* Only for AM625 OLDI TX */
+#define OLDI_PD_CTRL 0x100
+#define OLDI_LB_CTRL 0x104
+
#define OLDI_PWRDN_TX BIT(8)
+#define OLDI0_PWRDN_TX BIT(0)
+#define OLDI1_PWRDN_TX BIT(1)
#endif /* __TIDSS_DISPC_REGS_H */
--
2.37.0
Add support to enable and read the dss port properties.
The "ti,oldi-mode" property indicates the tidss driver how many OLDI
TXes are enabled as well as which mode do they need to be connected in.
The "ti,rgb565_to_888" is a special property that forces the DSS to
output 16bit RGB565 data to a 24bit RGB888 bridge. This property can be
used when the bridge does not explicity support RGB565.
Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/tidss/tidss_dispc.c | 55 ++++++++++++++++++++++++++---
drivers/gpu/drm/tidss/tidss_dispc.h | 8 +++++
2 files changed, 59 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index f084f0688a54..add725fa682b 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -348,6 +348,10 @@ struct dispc_device {
struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
+ enum dss_oldi_modes oldi_mode;
+
+ bool rgb565_to_888;
+
u32 *fourccs;
u32 num_fourccs;
@@ -2718,6 +2722,42 @@ static void dispc_softreset(struct dispc_device *dispc)
dev_warn(dispc->dev, "failed to reset dispc\n");
}
+static void dispc_get_port_properties(struct dispc_device *dispc)
+{
+ u32 i = 0;
+ struct device_node *dss_ports, *vport;
+
+ dss_ports = of_get_next_child(dispc->dev->of_node, NULL);
+
+ for_each_child_of_node(dss_ports, vport) {
+ if (dispc->feat->vp_bus_type[i] == DISPC_VP_OLDI) {
+ if (of_property_read_u32(vport, "ti,oldi-mode", &dispc->oldi_mode)) {
+ dev_dbg(dispc->dev, "%s: Using OLDI defaults on vp%d.\n",
+ __func__, i);
+ if (dispc->feat->subrev == DISPC_AM65X)
+ dispc->oldi_mode = OLDI_SINGLE_LINK_SINGLE_MODE_0;
+ else
+ dispc->oldi_mode = OLDI_MODE_OFF;
+ }
+
+ /*
+ * DISPC_AM65X DSS has a singular OLDI TX. It can not work in
+ * Dual/Duplicate Mode. Forcing defaults.
+ */
+ if (dispc->feat->subrev == DISPC_AM65X &&
+ dispc->oldi_mode > OLDI_SINGLE_LINK_SINGLE_MODE_0) {
+ dev_dbg(dispc->dev,
+ "%s: Using default OLDI mode %d. AM65X can not support mode %d.\n",
+ __func__, OLDI_SINGLE_LINK_SINGLE_MODE_0, dispc->oldi_mode);
+ dispc->oldi_mode = OLDI_SINGLE_LINK_SINGLE_MODE_0;
+ }
+ } else if (dispc->feat->vp_bus_type[i] == DISPC_VP_DPI) {
+ dispc->rgb565_to_888 = of_property_read_bool(vport, "ti,rgb565-to-888");
+ }
+ i++;
+ }
+}
+
int dispc_init(struct tidss_device *tidss)
{
struct device *dev = tidss->dev;
@@ -2812,10 +2852,17 @@ int dispc_init(struct tidss_device *tidss)
dispc->vp_data[i].gamma_table = gamma_table;
}
- if (feat->subrev == DISPC_AM65X) {
- r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
- if (r)
- return r;
+ if (feat->subrev == DISPC_AM65X || feat->subrev == DISPC_AM625) {
+ dispc_get_port_properties(dispc);
+ if (dispc->oldi_mode) {
+ r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
+ if (r)
+ return r;
+ }
+ } else {
+ /* K2G and J721E DSS do not support these properties */
+ dispc->oldi_mode = OLDI_MODE_OFF;
+ dispc->rgb565_to_888 = false;
}
dispc->fclk = devm_clk_get(dev, "fck");
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index a28005dafdc9..de8a95d3e3be 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 dss_oldi_modes {
+ OLDI_MODE_OFF, /* OLDI turned off / tied off in IP. */
+ OLDI_SINGLE_LINK_SINGLE_MODE_0, /* Single Output over OLDI 0. */
+ OLDI_SINGLE_LINK_SINGLE_MODE_1, /* Single Output over OLDI 1. */
+ OLDI_SINGLE_LINK_DUPLICATE_MODE, /* Duplicate Output over OLDI 0 and 1. */
+ OLDI_DUAL_LINK, /* Combined Output over OLDI 0 and 1. */
+};
+
struct dispc_features {
int min_pclk_khz;
int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
--
2.37.0
Override the DSS Videoport to output 16bit RGB565 instead of 24bit
RGB888 when the dt property "ti,rgb565-to-888" has been enanbled.
Co-developed-by: Darren Etheridge <[email protected]>
Signed-off-by: Darren Etheridge <[email protected]>
Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/tidss/tidss_dispc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index fb1fdecfc83a..94fce035c1d7 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -896,6 +896,8 @@ struct dispc_bus_format *dispc_vp_find_bus_fmt(struct dispc_device *dispc,
if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI &&
dispc->oldi_mode == OLDI_DUAL_LINK)
is_dual_link = true;
+ else if (bus_fmt == MEDIA_BUS_FMT_RGB888_1X24 && dispc->rgb565_to_888)
+ bus_fmt = MEDIA_BUS_FMT_RGB565_1X16;
for (i = 0; i < ARRAY_SIZE(dispc_bus_formats); ++i) {
if (dispc_bus_formats[i].bus_fmt == bus_fmt &&
--
2.37.0
On Tue, Jul 19, 2022 at 01:38:39PM +0530, Aradhya Bhatia wrote:
> Add am625-io-ctrl dt property to provide access to the control MMR
> registers for the OLDI TXes.
>
> These registers are used to control the power input to the OLDI TXes as
> well as to configure them in the Loopback test mode.
>
> The MMR IO controller device has been updated since the AM65x SoC and
> hence a newer property is needed to describe the one in AM625 SoC.
>
> Signed-off-by: Aradhya Bhatia <[email protected]>
> ---
> .../bindings/display/ti/ti,am65x-dss.yaml | 21 +++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> index 11d9b3821409..672765ad1f30 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> @@ -118,12 +118,33 @@ properties:
> and OLDI_CLK_IO_CTRL registers. This property is needed for OLDI
> interface to work.
>
> + ti,am625-oldi-io-ctrl:
> + $ref: "/schemas/types.yaml#/definitions/phandle"
> + description:
> + phandle to syscon device node mapping OLDI IO_CTRL registers, for
> + AM625 SoC. The mapped range should point to OLDI0_DAT0_IO_CTRL,
> + and map the registers up till OLDI_LB_CTRL. This property allows
> + the driver to control the power delivery to the OLDI TXes and
> + their loopback control as well.
What's wrong with the existing ti,am65x-oldi-io-ctrl other than the less
than ideal naming? And you just continued with the same issue so the
next part will need yet another property. Sorry, no. Just use the
existing property.
> +
> max-memory-bandwidth:
> $ref: /schemas/types.yaml#/definitions/uint32
> description:
> Input memory (from main memory to dispc) bandwidth limit in
> bytes per second
>
> +if:
> + properties:
> + compatible:
> + contains:
> + const: ti,am65x-dss
> +then:
> + properties:
> + ti,am625-oldi-io-ctrl: false
> +else:
> + properties:
> + ti,am65x-oldi-io-ctrl: false
> +
> required:
> - compatible
> - reg
> --
> 2.37.0
>
>
On 21-Jul-22 05:02, Rob Herring wrote:
> On Tue, Jul 19, 2022 at 01:38:39PM +0530, Aradhya Bhatia wrote:
>> Add am625-io-ctrl dt property to provide access to the control MMR
>> registers for the OLDI TXes.
>>
>> These registers are used to control the power input to the OLDI TXes as
>> well as to configure them in the Loopback test mode.
>>
>> The MMR IO controller device has been updated since the AM65x SoC and
>> hence a newer property is needed to describe the one in AM625 SoC.
>>
>> Signed-off-by: Aradhya Bhatia <[email protected]>
>> ---
>> .../bindings/display/ti/ti,am65x-dss.yaml | 21 +++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> index 11d9b3821409..672765ad1f30 100644
>> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> @@ -118,12 +118,33 @@ properties:
>> and OLDI_CLK_IO_CTRL registers. This property is needed for OLDI
>> interface to work.
>>
>> + ti,am625-oldi-io-ctrl:
>> + $ref: "/schemas/types.yaml#/definitions/phandle"
>> + description:
>> + phandle to syscon device node mapping OLDI IO_CTRL registers, for
>> + AM625 SoC. The mapped range should point to OLDI0_DAT0_IO_CTRL,
>> + and map the registers up till OLDI_LB_CTRL. This property allows
>> + the driver to control the power delivery to the OLDI TXes and
>> + their loopback control as well.
>
> What's wrong with the existing ti,am65x-oldi-io-ctrl other than the less
> than ideal naming? And you just continued with the same issue so the
> next part will need yet another property. Sorry, no. Just use the
> existing property.
>
I introduced the new property because the peripheral was a newer and
different implementation from the previous one.
However, the same property can be re-used. I will do so in the re-spin.
>> +
>> max-memory-bandwidth:
>> $ref: /schemas/types.yaml#/definitions/uint32
>> description:
>> Input memory (from main memory to dispc) bandwidth limit in
>> bytes per second
>>
>> +if:
>> + properties:
>> + compatible:
>> + contains:
>> + const: ti,am65x-dss
>> +then:
>> + properties:
>> + ti,am625-oldi-io-ctrl: false
>> +else:
>> + properties:
>> + ti,am65x-oldi-io-ctrl: false
>> +
>> required:
>> - compatible
>> - reg
>> --
>> 2.37.0
>>
>>
Regards
Aradhya
Hi,
On 19/07/2022 11:08, Aradhya Bhatia wrote:
> The AM625 DSS peripheral supports 2 OLDI TXes which can work to enable 2
> duplicated displays of smaller resolutions or enable a single Dual-Link
> display with a higher resolution (1920x1200).
>
> Configure the necessary register to enable the different modes.
>
> Signed-off-by: Aradhya Bhatia <[email protected]>
> ---
> drivers/gpu/drm/tidss/tidss_dispc.c | 44 +++++++++++++++++++++++++++--
> 1 file changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 0b9689453ee8..28cb61259471 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -1021,8 +1021,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
> + * set statically to 0.
> */
>
> if (fmt->data_width == 24)
> @@ -1039,7 +1039,45 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
>
> oldi_cfg |= BIT(0); /* ENABLE */
>
> - dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> + /*
> + * As per all the current implementations of DSS, the OLDI TXes are present only on
> + * hw_videoport = 0 (OLDI TX 0). However, the config register for 2nd OLDI TX (OLDI TX 1)
> + * is present in the address space of hw_videoport = 1. Hence, using "hw_videoport + 1" to
> + * configure OLDI TX 1.
> + */
> +
> + switch (dispc->oldi_mode) {
> + case OLDI_MODE_OFF:
> + oldi_cfg &= ~BIT(0); /* DISABLE */
> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> + dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> + break;
> +
> + case OLDI_SINGLE_LINK_SINGLE_MODE_0:
> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> + break;
> +
> + case OLDI_SINGLE_LINK_SINGLE_MODE_1:
> + dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> + break;
> +
> + case OLDI_SINGLE_LINK_DUPLICATE_MODE:
> + oldi_cfg |= BIT(5); /* DUPLICATE MODE */
> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> + dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> + break;
> +
> + case OLDI_DUAL_LINK:
> + oldi_cfg |= BIT(11); /* DUALMODESYNC */
> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> + dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> + break;
> +
> + default:
> + dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
> + __func__);
> + return;
> + }
>
> while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
> count < 10000)
This feels a bit hacky:
- The function is dispc_enable_oldi, but the above code also disables
oldi. We have code in dispc_vp_unprepare() which disables OLDI at the
moment.
- The function takes hw_videoport as a parameter, and is designed to
work on that videoport. The above operates on two videoports. Isn't the
function also called for hw_videoport +1, which would result in reg
writes to hw_videoport + 2?
- No matching code in dispc_vp_unprepare
Obviously the duplicate mode (I presume that's "cloning") and the dual
link complicate things here, and I have to say I haven't worked with
such setups. But I think somehow this should be restructured so that
common configuration (common to the OLDIs) is done somewhere else.
I would guess that there are other drivers that support cloning and dual
mode. Did you have a look how they handle things?
Tomi
On 27/07/2022 16:22, Tomi Valkeinen wrote:
> Hi,
>
> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>> The AM625 DSS peripheral supports 2 OLDI TXes which can work to enable 2
>> duplicated displays of smaller resolutions or enable a single Dual-Link
>> display with a higher resolution (1920x1200).
>>
>> Configure the necessary register to enable the different modes.
>>
>> Signed-off-by: Aradhya Bhatia <[email protected]>
>> ---
>> drivers/gpu/drm/tidss/tidss_dispc.c | 44 +++++++++++++++++++++++++++--
>> 1 file changed, 41 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c
>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index 0b9689453ee8..28cb61259471 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -1021,8 +1021,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
>> + * set statically to 0.
>> */
>> if (fmt->data_width == 24)
>> @@ -1039,7 +1039,45 @@ static void dispc_enable_oldi(struct
>> dispc_device *dispc, u32 hw_videoport,
>> oldi_cfg |= BIT(0); /* ENABLE */
>> - dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG,
>> oldi_cfg);
>> + /*
>> + * As per all the current implementations of DSS, the OLDI TXes
>> are present only on
>> + * hw_videoport = 0 (OLDI TX 0). However, the config register for
>> 2nd OLDI TX (OLDI TX 1)
>> + * is present in the address space of hw_videoport = 1. Hence,
>> using "hw_videoport + 1" to
>> + * configure OLDI TX 1.
>> + */
>> +
>> + switch (dispc->oldi_mode) {
>> + case OLDI_MODE_OFF:
>> + oldi_cfg &= ~BIT(0); /* DISABLE */
>> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG,
>> oldi_cfg);
>> + dispc_vp_write(dispc, hw_videoport + 1,
>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>> + break;
>> +
>> + case OLDI_SINGLE_LINK_SINGLE_MODE_0:
>> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG,
>> oldi_cfg);
>> + break;
>> +
>> + case OLDI_SINGLE_LINK_SINGLE_MODE_1:
>> + dispc_vp_write(dispc, hw_videoport + 1,
>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>> + break;
>> +
>> + case OLDI_SINGLE_LINK_DUPLICATE_MODE:
>> + oldi_cfg |= BIT(5); /* DUPLICATE MODE */
>> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG,
>> oldi_cfg);
>> + dispc_vp_write(dispc, hw_videoport + 1,
>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>> + break;
>> +
>> + case OLDI_DUAL_LINK:
>> + oldi_cfg |= BIT(11); /* DUALMODESYNC */
>> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG,
>> oldi_cfg);
>> + dispc_vp_write(dispc, hw_videoport + 1,
>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>> + break;
>> +
>> + default:
>> + dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
>> + __func__);
>> + return;
>> + }
>> while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
>> count < 10000)
>
> This feels a bit hacky:
>
> - The function is dispc_enable_oldi, but the above code also disables
> oldi. We have code in dispc_vp_unprepare() which disables OLDI at the
> moment.
>
> - The function takes hw_videoport as a parameter, and is designed to
> work on that videoport. The above operates on two videoports. Isn't the
> function also called for hw_videoport +1, which would result in reg
> writes to hw_videoport + 2?
>
> - No matching code in dispc_vp_unprepare
>
> Obviously the duplicate mode (I presume that's "cloning") and the dual
> link complicate things here, and I have to say I haven't worked with
> such setups. But I think somehow this should be restructured so that
> common configuration (common to the OLDIs) is done somewhere else.
>
> I would guess that there are other drivers that support cloning and dual
> mode. Did you have a look how they handle things?
Oh, I see now... There's just one dss video port for OLDI, the same as
in am65x, but that single video port is now connected to two OLDI TXes.
And thus this function will only be called for the single video port.
But... The registers for the second OLDI are part of the second video
port (DPI) register block?
Tomi
Hi Tomi,
On 28-Jul-22 12:16, Tomi Valkeinen wrote:
> On 27/07/2022 16:22, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>>> The AM625 DSS peripheral supports 2 OLDI TXes which can work to enable 2
>>> duplicated displays of smaller resolutions or enable a single Dual-Link
>>> display with a higher resolution (1920x1200).
>>>
>>> Configure the necessary register to enable the different modes.
>>>
>>> Signed-off-by: Aradhya Bhatia <[email protected]>
>>> ---
>>> drivers/gpu/drm/tidss/tidss_dispc.c | 44 +++++++++++++++++++++++++++--
>>> 1 file changed, 41 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c
>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> index 0b9689453ee8..28cb61259471 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> @@ -1021,8 +1021,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
>>> + * set statically to 0.
>>> */
>>> if (fmt->data_width == 24)
>>> @@ -1039,7 +1039,45 @@ static void dispc_enable_oldi(struct
>>> dispc_device *dispc, u32 hw_videoport,
>>> oldi_cfg |= BIT(0); /* ENABLE */
>>> - dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG,
>>> oldi_cfg);
>>> + /*
>>> + * As per all the current implementations of DSS, the OLDI TXes
>>> are present only on
>>> + * hw_videoport = 0 (OLDI TX 0). However, the config register
>>> for 2nd OLDI TX (OLDI TX 1)
>>> + * is present in the address space of hw_videoport = 1. Hence,
>>> using "hw_videoport + 1" to
>>> + * configure OLDI TX 1.
>>> + */
>>> +
>>> + switch (dispc->oldi_mode) {
>>> + case OLDI_MODE_OFF:
>>> + oldi_cfg &= ~BIT(0); /* DISABLE */
>>> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG,
>>> oldi_cfg);
>>> + dispc_vp_write(dispc, hw_videoport + 1,
>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>> + break;
>>> +
>>> + case OLDI_SINGLE_LINK_SINGLE_MODE_0:
>>> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG,
>>> oldi_cfg);
>>> + break;
>>> +
>>> + case OLDI_SINGLE_LINK_SINGLE_MODE_1:
>>> + dispc_vp_write(dispc, hw_videoport + 1,
>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>> + break;
>>> +
>>> + case OLDI_SINGLE_LINK_DUPLICATE_MODE:
>>> + oldi_cfg |= BIT(5); /* DUPLICATE MODE */
>>> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG,
>>> oldi_cfg);
>>> + dispc_vp_write(dispc, hw_videoport + 1,
>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>> + break;
>>> +
>>> + case OLDI_DUAL_LINK:
>>> + oldi_cfg |= BIT(11); /* DUALMODESYNC */
>>> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG,
>>> oldi_cfg);
>>> + dispc_vp_write(dispc, hw_videoport + 1,
>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>> + break;
>>> +
>>> + default:
>>> + dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
>>> + __func__);
>>> + return;
>>> + }
>>> while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
>>> count < 10000)
>>
>> This feels a bit hacky:
>>
>> - The function is dispc_enable_oldi, but the above code also disables
>> oldi. We have code in dispc_vp_unprepare() which disables OLDI at the
>> moment.
>>
>> - The function takes hw_videoport as a parameter, and is designed to
>> work on that videoport. The above operates on two videoports. Isn't
>> the function also called for hw_videoport +1, which would result in
>> reg writes to hw_videoport + 2?
>>
>> - No matching code in dispc_vp_unprepare
>>
>> Obviously the duplicate mode (I presume that's "cloning") and the dual
>> link complicate things here, and I have to say I haven't worked with
>> such setups. But I think somehow this should be restructured so that
>> common configuration (common to the OLDIs) is done somewhere else.
>>
>> I would guess that there are other drivers that support cloning and
>> dual mode. Did you have a look how they handle things?
>
> Oh, I see now... There's just one dss video port for OLDI, the same as
> in am65x, but that single video port is now connected to two OLDI TXes.
> And thus this function will only be called for the single video port.
> > But... The registers for the second OLDI are part of the second video
> port (DPI) register block?
Yes! The config register for the second OLDI TX has been (incorrectly)
added in the register space for the DPI video port. 'dispc_vp_prepare'
is the only function calling 'dispc_enable_oldi', and
'dispc_enable_oldi' would not be called for hw_videoports = 1 (DPI)
because of the conditional check.
Hence, to activate both the OLDI-TXes connected to the OLDI video port,
I had to use the (hw_videoport + 1) way.
However, I will remove the disable part from the 'dispc_enable_oldi' and
I will implement the disabling properly under 'dispc_vp_unprepare', in
the next version.
Regards
Aradhya
On 19/07/2022 11:08, Aradhya Bhatia wrote:
> The OLDI TX(es) require a serial clock which is 7 times the pixel clock
> of the display panel. When the OLDI is enabled in DSS, the pixel clock
> input of the corresponding videoport gets a divided-by 7 value of the
> requested clock.
>
> For the am625-dss, the requested clock needs to be 7 times the value.
>
> Update the clock frequency by requesting 7 times the value.
>
> Signed-off-by: Aradhya Bhatia <[email protected]>
> ---
> drivers/gpu/drm/tidss/tidss_dispc.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index c4a5f808648f..0b9689453ee8 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -1326,6 +1326,16 @@ int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
> int r;
> unsigned long new_rate;
>
> + /*
> + * For AM625 OLDI video ports, the requested pixel clock needs to take into account the
> + * serial clock required for the serialization of DPI signals into LVDS signals. The
> + * incoming pixel clock on the OLDI video port gets divided by 7 whenever OLDI enable bit
> + * gets set.
> + */
> + if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI &&
> + dispc->feat->subrev == DISPC_AM625)
> + rate *= 7;
> +
> r = clk_set_rate(dispc->vp_clk[hw_videoport], rate);
> if (r) {
> dev_err(dispc->dev, "vp%d: failed to set clk rate to %lu\n",
The AM625 TRM seems to be missing the "DSS integration" section, even if
it's referred to in three places in the TRM. Supposedly that has details
about the clocking.
Shouldn't the source clock be 3.5x when dual-link mode is used?
While I don't know the details, this doesn't feel correct. We're
supposed to be setting the VP pixel clock here, and the serial clock
would be derived from that as it's done on AM65x. Is the DT clock tree
wrong for AM625?
Tomi
On 19/07/2022 11:08, Aradhya Bhatia wrote:
> The 2 OLDI TXes in the AM625 SoC can be synced together to output a 2K
> resolution video.
>
> Add support in the driver for the discovery of such a dual mode
> connection on the OLDI video port, using the values of "ti,oldi-mode"
> property.
>
> Signed-off-by: Aradhya Bhatia <[email protected]>
> ---
> drivers/gpu/drm/tidss/tidss_dispc.c | 39 +++++++++++++++++++++--------
> 1 file changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index add725fa682b..fb1fdecfc83a 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -853,25 +853,36 @@ void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
> }
> }
>
> -enum dispc_oldi_mode_reg_val { SPWG_18 = 0, JEIDA_24 = 1, SPWG_24 = 2 };
> +enum dispc_oldi_mode_reg_val {
> + SPWG_18 = 0,
> + JEIDA_24 = 1,
> + SPWG_24 = 2,
> + DL_SPWG_18 = 4,
> + DL_JEIDA_24 = 5,
> + DL_SPWG_24 = 6,
> +};
>
> struct dispc_bus_format {
> u32 bus_fmt;
> u32 data_width;
> bool is_oldi_fmt;
> + bool is_dual_link;
> enum dispc_oldi_mode_reg_val oldi_mode_reg_val;
> };
>
> static const struct dispc_bus_format dispc_bus_formats[] = {
> - { MEDIA_BUS_FMT_RGB444_1X12, 12, false, 0 },
> - { MEDIA_BUS_FMT_RGB565_1X16, 16, false, 0 },
> - { MEDIA_BUS_FMT_RGB666_1X18, 18, false, 0 },
> - { MEDIA_BUS_FMT_RGB888_1X24, 24, false, 0 },
> - { MEDIA_BUS_FMT_RGB101010_1X30, 30, false, 0 },
> - { MEDIA_BUS_FMT_RGB121212_1X36, 36, false, 0 },
> - { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, SPWG_18 },
> - { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, SPWG_24 },
> - { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, JEIDA_24 },
> + { MEDIA_BUS_FMT_RGB444_1X12, 12, false, false, 0 },
> + { MEDIA_BUS_FMT_RGB565_1X16, 16, false, false, 0 },
> + { MEDIA_BUS_FMT_RGB666_1X18, 18, false, false, 0 },
> + { MEDIA_BUS_FMT_RGB888_1X24, 24, false, false, 0 },
> + { MEDIA_BUS_FMT_RGB101010_1X30, 30, false, false, 0 },
> + { MEDIA_BUS_FMT_RGB121212_1X36, 36, false, false, 0 },
> + { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, false, SPWG_18 },
> + { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, false, SPWG_24 },
> + { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, false, JEIDA_24 },
> + { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, true, DL_SPWG_18 },
> + { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, true, DL_SPWG_24 },
> + { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, true, DL_JEIDA_24 },
> };
So the dual link sends two pixels per clock, right? Are there panel or
bridge drivers that support this? My initial thought was that it should
be a new bus format.
Tomi
On 28/07/2022 11:49, Aradhya Bhatia wrote:
> Hi Tomi,
>
> On 28-Jul-22 12:16, Tomi Valkeinen wrote:
>> On 27/07/2022 16:22, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>>>> The AM625 DSS peripheral supports 2 OLDI TXes which can work to
>>>> enable 2
>>>> duplicated displays of smaller resolutions or enable a single Dual-Link
>>>> display with a higher resolution (1920x1200).
>>>>
>>>> Configure the necessary register to enable the different modes.
>>>>
>>>> Signed-off-by: Aradhya Bhatia <[email protected]>
>>>> ---
>>>> drivers/gpu/drm/tidss/tidss_dispc.c | 44
>>>> +++++++++++++++++++++++++++--
>>>> 1 file changed, 41 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> index 0b9689453ee8..28cb61259471 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> @@ -1021,8 +1021,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
>>>> + * set statically to 0.
>>>> */
>>>> if (fmt->data_width == 24)
>>>> @@ -1039,7 +1039,45 @@ static void dispc_enable_oldi(struct
>>>> dispc_device *dispc, u32 hw_videoport,
>>>> oldi_cfg |= BIT(0); /* ENABLE */
>>>> - dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG,
>>>> oldi_cfg);
>>>> + /*
>>>> + * As per all the current implementations of DSS, the OLDI TXes
>>>> are present only on
>>>> + * hw_videoport = 0 (OLDI TX 0). However, the config register
>>>> for 2nd OLDI TX (OLDI TX 1)
>>>> + * is present in the address space of hw_videoport = 1. Hence,
>>>> using "hw_videoport + 1" to
>>>> + * configure OLDI TX 1.
>>>> + */
>>>> +
>>>> + switch (dispc->oldi_mode) {
>>>> + case OLDI_MODE_OFF:
>>>> + oldi_cfg &= ~BIT(0); /* DISABLE */
>>>> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG,
>>>> oldi_cfg);
>>>> + dispc_vp_write(dispc, hw_videoport + 1,
>>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>>> + break;
>>>> +
>>>> + case OLDI_SINGLE_LINK_SINGLE_MODE_0:
>>>> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG,
>>>> oldi_cfg);
>>>> + break;
>>>> +
>>>> + case OLDI_SINGLE_LINK_SINGLE_MODE_1:
>>>> + dispc_vp_write(dispc, hw_videoport + 1,
>>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>>> + break;
>>>> +
>>>> + case OLDI_SINGLE_LINK_DUPLICATE_MODE:
>>>> + oldi_cfg |= BIT(5); /* DUPLICATE MODE */
>>>> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG,
>>>> oldi_cfg);
>>>> + dispc_vp_write(dispc, hw_videoport + 1,
>>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>>> + break;
>>>> +
>>>> + case OLDI_DUAL_LINK:
>>>> + oldi_cfg |= BIT(11); /* DUALMODESYNC */
>>>> + dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG,
>>>> oldi_cfg);
>>>> + dispc_vp_write(dispc, hw_videoport + 1,
>>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>>> + break;
>>>> +
>>>> + default:
>>>> + dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
>>>> + __func__);
>>>> + return;
>>>> + }
>>>> while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
>>>> count < 10000)
>>>
>>> This feels a bit hacky:
>>>
>>> - The function is dispc_enable_oldi, but the above code also disables
>>> oldi. We have code in dispc_vp_unprepare() which disables OLDI at the
>>> moment.
>>>
>>> - The function takes hw_videoport as a parameter, and is designed to
>>> work on that videoport. The above operates on two videoports. Isn't
>>> the function also called for hw_videoport +1, which would result in
>>> reg writes to hw_videoport + 2?
>>>
>>> - No matching code in dispc_vp_unprepare
>>>
>>> Obviously the duplicate mode (I presume that's "cloning") and the
>>> dual link complicate things here, and I have to say I haven't worked
>>> with such setups. But I think somehow this should be restructured so
>>> that common configuration (common to the OLDIs) is done somewhere else.
>>>
>>> I would guess that there are other drivers that support cloning and
>>> dual mode. Did you have a look how they handle things?
>>
>> Oh, I see now... There's just one dss video port for OLDI, the same as
>> in am65x, but that single video port is now connected to two OLDI
>> TXes. And thus this function will only be called for the single video
>> port.
>> > But... The registers for the second OLDI are part of the second video
>> port (DPI) register block?
>
> Yes! The config register for the second OLDI TX has been (incorrectly)
> added in the register space for the DPI video port. 'dispc_vp_prepare'
> is the only function calling 'dispc_enable_oldi', and
> 'dispc_enable_oldi' would not be called for hw_videoports = 1 (DPI)
> because of the conditional check.
>
> Hence, to activate both the OLDI-TXes connected to the OLDI video port,
> I had to use the (hw_videoport + 1) way.
I think this should be highlighted in the comment more clearly. Also, I
don't think hw_videoport + 1 usage is good. While in this case the
registers are in vp + 1, it's, in a way, a coincidence.
Instead, have a new variable for the VP register block which contains
the OLDI TX 1 registers, and just set that to 1 with a comment
clarifying that the registers for OLDI 1 are in VP1 register block, even
if OLDI 1 is connected to VP0.
And then use that variable when calling dispc_vp_write().
Tomi
On 28/07/2022 14:03, Tomi Valkeinen wrote:
> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>> The 2 OLDI TXes in the AM625 SoC can be synced together to output a 2K
>> resolution video.
>>
>> Add support in the driver for the discovery of such a dual mode
>> connection on the OLDI video port, using the values of "ti,oldi-mode"
>> property.
>>
>> Signed-off-by: Aradhya Bhatia <[email protected]>
>> ---
>> drivers/gpu/drm/tidss/tidss_dispc.c | 39 +++++++++++++++++++++--------
>> 1 file changed, 28 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c
>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index add725fa682b..fb1fdecfc83a 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -853,25 +853,36 @@ void dispc_set_irqenable(struct dispc_device
>> *dispc, dispc_irq_t mask)
>> }
>> }
>> -enum dispc_oldi_mode_reg_val { SPWG_18 = 0, JEIDA_24 = 1, SPWG_24 = 2 };
>> +enum dispc_oldi_mode_reg_val {
>> + SPWG_18 = 0,
>> + JEIDA_24 = 1,
>> + SPWG_24 = 2,
>> + DL_SPWG_18 = 4,
>> + DL_JEIDA_24 = 5,
>> + DL_SPWG_24 = 6,
>> +};
>> struct dispc_bus_format {
>> u32 bus_fmt;
>> u32 data_width;
>> bool is_oldi_fmt;
>> + bool is_dual_link;
>> enum dispc_oldi_mode_reg_val oldi_mode_reg_val;
>> };
>> static const struct dispc_bus_format dispc_bus_formats[] = {
>> - { MEDIA_BUS_FMT_RGB444_1X12, 12, false, 0 },
>> - { MEDIA_BUS_FMT_RGB565_1X16, 16, false, 0 },
>> - { MEDIA_BUS_FMT_RGB666_1X18, 18, false, 0 },
>> - { MEDIA_BUS_FMT_RGB888_1X24, 24, false, 0 },
>> - { MEDIA_BUS_FMT_RGB101010_1X30, 30, false, 0 },
>> - { MEDIA_BUS_FMT_RGB121212_1X36, 36, false, 0 },
>> - { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, SPWG_18 },
>> - { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, SPWG_24 },
>> - { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, JEIDA_24 },
>> + { MEDIA_BUS_FMT_RGB444_1X12, 12, false, false, 0 },
>> + { MEDIA_BUS_FMT_RGB565_1X16, 16, false, false, 0 },
>> + { MEDIA_BUS_FMT_RGB666_1X18, 18, false, false, 0 },
>> + { MEDIA_BUS_FMT_RGB888_1X24, 24, false, false, 0 },
>> + { MEDIA_BUS_FMT_RGB101010_1X30, 30, false, false, 0 },
>> + { MEDIA_BUS_FMT_RGB121212_1X36, 36, false, false, 0 },
>> + { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, false, SPWG_18 },
>> + { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, false, SPWG_24 },
>> + { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, false, JEIDA_24 },
>> + { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, true, DL_SPWG_18 },
>> + { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, true, DL_SPWG_24 },
>> + { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, true,
>> DL_JEIDA_24 },
>> };
>
> So the dual link sends two pixels per clock, right? Are there panel or
> bridge drivers that support this? My initial thought was that it should
> be a new bus format.
Looks like we have drm bridges supporting dual link, and they use the
"normal" bus format. Did you have a look at them? They require two port
nodes for dual link, and use the existence of the second one to decide
if dual link is used or not.
There are also lvds helpers in drm_of.c. I didn't look closely, but it
looked to me that the helpers can tell you if the ports are connected to
a dual link bridge. If not, you could fall back to cloning. This way no
extra properties are needed. But you will need to add a port node, which
I think you need to add anyway for cloning.
Tomi
On 19/07/2022 11:08, Aradhya Bhatia wrote:
> Add support to enable and read the dss port properties.
>
> The "ti,oldi-mode" property indicates the tidss driver how many OLDI
> TXes are enabled as well as which mode do they need to be connected in.
>
> The "ti,rgb565_to_888" is a special property that forces the DSS to
> output 16bit RGB565 data to a 24bit RGB888 bridge. This property can be
> used when the bridge does not explicity support RGB565.
>
> Signed-off-by: Aradhya Bhatia <[email protected]>
I think it would be good if you would split the patches that contain
both OLDI and rgb565 changes, and arrange the series so that you first
have patches that add the rgb565 (dt and driver), and after that, the
OLDI changes.
They are fully separate things, and makes understanding the changes a
bit easier.
Tomi
On 28-Jul-22 15:35, Tomi Valkeinen wrote:
> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>> The OLDI TX(es) require a serial clock which is 7 times the pixel clock
>> of the display panel. When the OLDI is enabled in DSS, the pixel clock
>> input of the corresponding videoport gets a divided-by 7 value of the
>> requested clock.
>>
>> For the am625-dss, the requested clock needs to be 7 times the value.
>>
>> Update the clock frequency by requesting 7 times the value.
>>
>> Signed-off-by: Aradhya Bhatia <[email protected]>
>> ---
>> drivers/gpu/drm/tidss/tidss_dispc.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c
>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index c4a5f808648f..0b9689453ee8 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -1326,6 +1326,16 @@ int dispc_vp_set_clk_rate(struct dispc_device
>> *dispc, u32 hw_videoport,
>> int r;
>> unsigned long new_rate;
>> + /*
>> + * For AM625 OLDI video ports, the requested pixel clock needs to
>> take into account the
>> + * serial clock required for the serialization of DPI signals
>> into LVDS signals. The
>> + * incoming pixel clock on the OLDI video port gets divided by 7
>> whenever OLDI enable bit
>> + * gets set.
>> + */
>> + if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI &&
>> + dispc->feat->subrev == DISPC_AM625)
>> + rate *= 7;
>> +
>> r = clk_set_rate(dispc->vp_clk[hw_videoport], rate);
>> if (r) {
>> dev_err(dispc->dev, "vp%d: failed to set clk rate to %lu\n",
>
> The AM625 TRM seems to be missing the "DSS integration" section, even if
> it's referred to in three places in the TRM. Supposedly that has details
> about the clocking.
>
> Shouldn't the source clock be 3.5x when dual-link mode is used?
There should not be.
Whenever OLDI is enabled, the clock generated from the PLL is 7 times
the required pixel clock.
For the OLDI TXes, the clock passes through a /2 divider. This divider
only gets activated when the dual mode has been enabled in the OLDI
configuration. Thus the OLDI TXes get 3.5x the pixel clock in dual mode.
When the OLDI has been configured for a single mode,
the PLL clock passes through the /2 divider without any change.
>
> While I don't know the details, this doesn't feel correct. We're
> supposed to be setting the VP pixel clock here, and the serial clock
> would be derived from that as it's done on AM65x. Is the DT clock tree
> wrong for AM625?
Ideally, yes, its the pixel frequency that we are supposed to set here.
The same PLL clock (7 times the pixel frequency) passes through a /7
clock divider. This clock divider only gets active when OLDI is enabled.
Thus, the DSS VP clock input, only gets the actual pixel frequency that
it needs.
Since, the /7 divider is controlled by a signal from the DSS, the driver
needs to request 7 times more the pixel clock to accommodate for the
divider.
In AM65X, the system FW is able to model the 7 times requirement because
the divider is not controlled by the DSS signal. DSS signal controls a
multiplexer which receives both PLL Clock and PLL / 7 clock.
Regards
Aradhya
On 29/07/2022 06:56, Aradhya Bhatia wrote:
>
> On 28-Jul-22 15:35, Tomi Valkeinen wrote:
>> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>>> The OLDI TX(es) require a serial clock which is 7 times the pixel clock
>>> of the display panel. When the OLDI is enabled in DSS, the pixel clock
>>> input of the corresponding videoport gets a divided-by 7 value of the
>>> requested clock.
>>>
>>> For the am625-dss, the requested clock needs to be 7 times the value.
>>>
>>> Update the clock frequency by requesting 7 times the value.
>>>
>>> Signed-off-by: Aradhya Bhatia <[email protected]>
>>> ---
>>> drivers/gpu/drm/tidss/tidss_dispc.c | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c
>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> index c4a5f808648f..0b9689453ee8 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> @@ -1326,6 +1326,16 @@ int dispc_vp_set_clk_rate(struct dispc_device
>>> *dispc, u32 hw_videoport,
>>> int r;
>>> unsigned long new_rate;
>>> + /*
>>> + * For AM625 OLDI video ports, the requested pixel clock needs
>>> to take into account the
>>> + * serial clock required for the serialization of DPI signals
>>> into LVDS signals. The
>>> + * incoming pixel clock on the OLDI video port gets divided by 7
>>> whenever OLDI enable bit
>>> + * gets set.
>>> + */
>>> + if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI &&
>>> + dispc->feat->subrev == DISPC_AM625)
>>> + rate *= 7;
>>> +
>>> r = clk_set_rate(dispc->vp_clk[hw_videoport], rate);
>>> if (r) {
>>> dev_err(dispc->dev, "vp%d: failed to set clk rate to %lu\n",
>>
>> The AM625 TRM seems to be missing the "DSS integration" section, even
>> if it's referred to in three places in the TRM. Supposedly that has
>> details about the clocking.
>>
>> Shouldn't the source clock be 3.5x when dual-link mode is used?
> There should not be.
>
> Whenever OLDI is enabled, the clock generated from the PLL is 7 times
> the required pixel clock.
>
> For the OLDI TXes, the clock passes through a /2 divider. This divider
> only gets activated when the dual mode has been enabled in the OLDI
> configuration. Thus the OLDI TXes get 3.5x the pixel clock in dual mode.
>
> When the OLDI has been configured for a single mode,
> the PLL clock passes through the /2 divider without any change.
>
>>
>> While I don't know the details, this doesn't feel correct. We're
>> supposed to be setting the VP pixel clock here, and the serial clock
>> would be derived from that as it's done on AM65x. Is the DT clock tree
>> wrong for AM625?
> Ideally, yes, its the pixel frequency that we are supposed to set here.
>
> The same PLL clock (7 times the pixel frequency) passes through a /7
> clock divider. This clock divider only gets active when OLDI is enabled.
> Thus, the DSS VP clock input, only gets the actual pixel frequency that
> it needs.
>
> Since, the /7 divider is controlled by a signal from the DSS, the driver
> needs to request 7 times more the pixel clock to accommodate for the
> divider.
>
> In AM65X, the system FW is able to model the 7 times requirement because
> the divider is not controlled by the DSS signal. DSS signal controls a
> multiplexer which receives both PLL Clock and PLL / 7 clock.
Could you ping the TI TRM team to add the DSS integration chapter? The
clock tree is quite important part of the TRM.
I really don't like this one much. The dispc->vp_clk[0] here is actually
not VP clock, but a parent clock of the VP clock, so, afaics, the driver
gets "wrong" clock from DT data and then fixes things by manually
multiplying the requested rate. If we have to go with a driver hack like
this, I think it needs to be clarified more with comments, in the DT
data also.
On AM625, the first videoport is always connected to OLDI(s), and never
goes through as DPI, right? I.e. you cannot bypass OLDI?
If so, the /7 is, in practice, always enabled, as OLDI is the only use
case. So why not add /7 node to the DT clock data, which would allow us
to get rid of this code?
Tomi
On 28-Jul-22 16:33, Tomi Valkeinen wrote:
> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>> The 2 OLDI TXes in the AM625 SoC can be synced together to output a 2K
>> resolution video.
>>
>> Add support in the driver for the discovery of such a dual mode
>> connection on the OLDI video port, using the values of "ti,oldi-mode"
>> property.
>>
>> Signed-off-by: Aradhya Bhatia <[email protected]>
>> ---
>> drivers/gpu/drm/tidss/tidss_dispc.c | 39 +++++++++++++++++++++--------
>> 1 file changed, 28 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c
>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index add725fa682b..fb1fdecfc83a 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -853,25 +853,36 @@ void dispc_set_irqenable(struct dispc_device
>> *dispc, dispc_irq_t mask)
>> }
>> }
>> -enum dispc_oldi_mode_reg_val { SPWG_18 = 0, JEIDA_24 = 1, SPWG_24 = 2 };
>> +enum dispc_oldi_mode_reg_val {
>> + SPWG_18 = 0,
>> + JEIDA_24 = 1,
>> + SPWG_24 = 2,
>> + DL_SPWG_18 = 4,
>> + DL_JEIDA_24 = 5,
>> + DL_SPWG_24 = 6,
>> +};
>> struct dispc_bus_format {
>> u32 bus_fmt;
>> u32 data_width;
>> bool is_oldi_fmt;
>> + bool is_dual_link;
>> enum dispc_oldi_mode_reg_val oldi_mode_reg_val;
>> };
>> static const struct dispc_bus_format dispc_bus_formats[] = {
>> - { MEDIA_BUS_FMT_RGB444_1X12, 12, false, 0 },
>> - { MEDIA_BUS_FMT_RGB565_1X16, 16, false, 0 },
>> - { MEDIA_BUS_FMT_RGB666_1X18, 18, false, 0 },
>> - { MEDIA_BUS_FMT_RGB888_1X24, 24, false, 0 },
>> - { MEDIA_BUS_FMT_RGB101010_1X30, 30, false, 0 },
>> - { MEDIA_BUS_FMT_RGB121212_1X36, 36, false, 0 },
>> - { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, SPWG_18 },
>> - { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, SPWG_24 },
>> - { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, JEIDA_24 },
>> + { MEDIA_BUS_FMT_RGB444_1X12, 12, false, false, 0 },
>> + { MEDIA_BUS_FMT_RGB565_1X16, 16, false, false, 0 },
>> + { MEDIA_BUS_FMT_RGB666_1X18, 18, false, false, 0 },
>> + { MEDIA_BUS_FMT_RGB888_1X24, 24, false, false, 0 },
>> + { MEDIA_BUS_FMT_RGB101010_1X30, 30, false, false, 0 },
>> + { MEDIA_BUS_FMT_RGB121212_1X36, 36, false, false, 0 },
>> + { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, false, SPWG_18 },
>> + { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, false, SPWG_24 },
>> + { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, false, JEIDA_24 },
>> + { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, true, DL_SPWG_18 },
>> + { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, true, DL_SPWG_24 },
>> + { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, true,
>> DL_JEIDA_24 },
>> };
>
> So the dual link sends two pixels per clock, right? Are there panel or
> bridge drivers that support this? My initial thought was that it should
> be a new bus format.
In dual link, we are having 2 OLDI TXes simultaneously send pixels, at a
fraction of the pixel frequency clock. Both the TXes have their own
clock lanes and they are in sync.
At the moment, we are not modeling the OLDI TXes as bridges in the DT,
nor are the drivers for these written. The tidss driver handles the
configuration, as the register is inside the DSS video ports address
space.
The need to add a dual link field in the above patch is there because
the OLDI config registers needs to know so. The output from both the
TXes remains according to the standard bus formats.
Regards
Aradhya
Hi Tomi,
On 28-Jul-22 17:15, Tomi Valkeinen wrote:
> On 28/07/2022 14:03, Tomi Valkeinen wrote:
>> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>>> The 2 OLDI TXes in the AM625 SoC can be synced together to output a 2K
>>> resolution video.
>>>
>>> Add support in the driver for the discovery of such a dual mode
>>> connection on the OLDI video port, using the values of "ti,oldi-mode"
>>> property.
>>>
>>> Signed-off-by: Aradhya Bhatia <[email protected]>
>>> ---
>>> drivers/gpu/drm/tidss/tidss_dispc.c | 39 +++++++++++++++++++++--------
>>> 1 file changed, 28 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c
>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> index add725fa682b..fb1fdecfc83a 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> @@ -853,25 +853,36 @@ void dispc_set_irqenable(struct dispc_device
>>> *dispc, dispc_irq_t mask)
>>> }
>>> }
>>> -enum dispc_oldi_mode_reg_val { SPWG_18 = 0, JEIDA_24 = 1, SPWG_24 =
>>> 2 };
>>> +enum dispc_oldi_mode_reg_val {
>>> + SPWG_18 = 0,
>>> + JEIDA_24 = 1,
>>> + SPWG_24 = 2,
>>> + DL_SPWG_18 = 4,
>>> + DL_JEIDA_24 = 5,
>>> + DL_SPWG_24 = 6,
>>> +};
>>> struct dispc_bus_format {
>>> u32 bus_fmt;
>>> u32 data_width;
>>> bool is_oldi_fmt;
>>> + bool is_dual_link;
>>> enum dispc_oldi_mode_reg_val oldi_mode_reg_val;
>>> };
>>> static const struct dispc_bus_format dispc_bus_formats[] = {
>>> - { MEDIA_BUS_FMT_RGB444_1X12, 12, false, 0 },
>>> - { MEDIA_BUS_FMT_RGB565_1X16, 16, false, 0 },
>>> - { MEDIA_BUS_FMT_RGB666_1X18, 18, false, 0 },
>>> - { MEDIA_BUS_FMT_RGB888_1X24, 24, false, 0 },
>>> - { MEDIA_BUS_FMT_RGB101010_1X30, 30, false, 0 },
>>> - { MEDIA_BUS_FMT_RGB121212_1X36, 36, false, 0 },
>>> - { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, SPWG_18 },
>>> - { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, SPWG_24 },
>>> - { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, JEIDA_24 },
>>> + { MEDIA_BUS_FMT_RGB444_1X12, 12, false, false, 0 },
>>> + { MEDIA_BUS_FMT_RGB565_1X16, 16, false, false, 0 },
>>> + { MEDIA_BUS_FMT_RGB666_1X18, 18, false, false, 0 },
>>> + { MEDIA_BUS_FMT_RGB888_1X24, 24, false, false, 0 },
>>> + { MEDIA_BUS_FMT_RGB101010_1X30, 30, false, false, 0 },
>>> + { MEDIA_BUS_FMT_RGB121212_1X36, 36, false, false, 0 },
>>> + { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, false, SPWG_18 },
>>> + { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, false, SPWG_24 },
>>> + { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, false, JEIDA_24 },
>>> + { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, true, DL_SPWG_18 },
>>> + { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, true, DL_SPWG_24 },
>>> + { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, true, DL_JEIDA_24 },
>>> };
>>
>> So the dual link sends two pixels per clock, right? Are there panel or
>> bridge drivers that support this? My initial thought was that it
>> should be a new bus format.
>
> Looks like we have drm bridges supporting dual link, and they use the
> "normal" bus format. Did you have a look at them? They require two port
> nodes for dual link, and use the existence of the second one to decide
> if dual link is used or not.
The above edits were not for adding a new bus format for dual link
connections. I added them in order to be able to write the correct OLDI
config values in the register.
>
> There are also lvds helpers in drm_of.c. I didn't look closely, but it
> looked to me that the helpers can tell you if the ports are connected to
> a dual link bridge. If not, you could fall back to cloning. This way no
> extra properties are needed. But you will need to add a port node, which
> I think you need to add anyway for cloning.
I have now seen drm_of.c and examples (renesas' rcar lvds) that use the
apis that drm_of.c is offering. In those cases, the OLDI TXes are being
modeled as separate devices, which is not the case with the tidss' OLDI
TXes. Since the only few OLDI registers are in the DSS address space,
they were just being configured through the tidss driver.
Even in DT, the dss port (for OLDI) connects to the panel port's
endpoint directly. Even in cases of dual link or cloning, it's only a
singular remote-to-endpoint connection between the (OLDI) VP and the
panel port. Hence the requirement of the properties in the earlier
patches of the series.
The use of lvds helper functions does not seem feasible in this case,
because even they read DT properties to determine the dual link
connection and those properties need to be a part of a lvds bridge
device.
I have also been considering the idea of implementing a new device
driver for the OLDI TXes, not unlike the renesas' one. That way the
driver could have the properties and the lvds helper functions at their
disposal. I am just slightly unsure if that would allow space for any
conflicts because of the shared register space.
Do let me know what you think!
Regards
Aradhya
Hi,
On 09/08/2022 08:58, Aradhya Bhatia wrote:
> Hi Tomi,
>
> On 28-Jul-22 17:15, Tomi Valkeinen wrote:
>> On 28/07/2022 14:03, Tomi Valkeinen wrote:
>>> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>>>> The 2 OLDI TXes in the AM625 SoC can be synced together to output a 2K
>>>> resolution video.
>>>>
>>>> Add support in the driver for the discovery of such a dual mode
>>>> connection on the OLDI video port, using the values of "ti,oldi-mode"
>>>> property.
>>>>
>>>> Signed-off-by: Aradhya Bhatia <[email protected]>
>>>> ---
>>>> drivers/gpu/drm/tidss/tidss_dispc.c | 39
>>>> +++++++++++++++++++++--------
>>>> 1 file changed, 28 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> index add725fa682b..fb1fdecfc83a 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> @@ -853,25 +853,36 @@ void dispc_set_irqenable(struct dispc_device
>>>> *dispc, dispc_irq_t mask)
>>>> }
>>>> }
>>>> -enum dispc_oldi_mode_reg_val { SPWG_18 = 0, JEIDA_24 = 1, SPWG_24 =
>>>> 2 };
>>>> +enum dispc_oldi_mode_reg_val {
>>>> + SPWG_18 = 0,
>>>> + JEIDA_24 = 1,
>>>> + SPWG_24 = 2,
>>>> + DL_SPWG_18 = 4,
>>>> + DL_JEIDA_24 = 5,
>>>> + DL_SPWG_24 = 6,
>>>> +};
>>>> struct dispc_bus_format {
>>>> u32 bus_fmt;
>>>> u32 data_width;
>>>> bool is_oldi_fmt;
>>>> + bool is_dual_link;
>>>> enum dispc_oldi_mode_reg_val oldi_mode_reg_val;
>>>> };
>>>> static const struct dispc_bus_format dispc_bus_formats[] = {
>>>> - { MEDIA_BUS_FMT_RGB444_1X12, 12, false, 0 },
>>>> - { MEDIA_BUS_FMT_RGB565_1X16, 16, false, 0 },
>>>> - { MEDIA_BUS_FMT_RGB666_1X18, 18, false, 0 },
>>>> - { MEDIA_BUS_FMT_RGB888_1X24, 24, false, 0 },
>>>> - { MEDIA_BUS_FMT_RGB101010_1X30, 30, false, 0 },
>>>> - { MEDIA_BUS_FMT_RGB121212_1X36, 36, false, 0 },
>>>> - { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, SPWG_18 },
>>>> - { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, SPWG_24 },
>>>> - { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, JEIDA_24 },
>>>> + { MEDIA_BUS_FMT_RGB444_1X12, 12, false, false, 0 },
>>>> + { MEDIA_BUS_FMT_RGB565_1X16, 16, false, false, 0 },
>>>> + { MEDIA_BUS_FMT_RGB666_1X18, 18, false, false, 0 },
>>>> + { MEDIA_BUS_FMT_RGB888_1X24, 24, false, false, 0 },
>>>> + { MEDIA_BUS_FMT_RGB101010_1X30, 30, false, false, 0 },
>>>> + { MEDIA_BUS_FMT_RGB121212_1X36, 36, false, false, 0 },
>>>> + { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, false, SPWG_18 },
>>>> + { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, false, SPWG_24 },
>>>> + { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, false,
>>>> JEIDA_24 },
>>>> + { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, true,
>>>> DL_SPWG_18 },
>>>> + { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, true,
>>>> DL_SPWG_24 },
>>>> + { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, true,
>>>> DL_JEIDA_24 },
>>>> };
>>>
>>> So the dual link sends two pixels per clock, right? Are there panel
>>> or bridge drivers that support this? My initial thought was that it
>>> should be a new bus format.
>>
>> Looks like we have drm bridges supporting dual link, and they use the
>> "normal" bus format. Did you have a look at them? They require two
>> port nodes for dual link, and use the existence of the second one to
>> decide if dual link is used or not.
> The above edits were not for adding a new bus format for dual link
> connections. I added them in order to be able to write the correct OLDI
> config values in the register.
>
>>
>> There are also lvds helpers in drm_of.c. I didn't look closely, but it
>> looked to me that the helpers can tell you if the ports are connected
>> to a dual link bridge. If not, you could fall back to cloning. This
>> way no extra properties are needed. But you will need to add a port
>> node, which I think you need to add anyway for cloning.
> I have now seen drm_of.c and examples (renesas' rcar lvds) that use the
> apis that drm_of.c is offering. In those cases, the OLDI TXes are being
> modeled as separate devices, which is not the case with the tidss' OLDI
> TXes. Since the only few OLDI registers are in the DSS address space,
> they were just being configured through the tidss driver.
I think it's irrelevant (in the bigger picture) whether the TXes are
separate devices, single device or part of some other device. Or why do
you think it matters?
> Even in DT, the dss port (for OLDI) connects to the panel port's
> endpoint directly. Even in cases of dual link or cloning, it's only a
> singular remote-to-endpoint connection between the (OLDI) VP and the
> panel port. Hence the requirement of the properties in the earlier
> patches of the series.
Sorry, I don't follow. If you use cloning, you have two TX outputs,
going to two panels, right? So you need two panel DT nodes, and those
would connect to two OLDI TX ports in the DSS.
Afaics the existing dual link bridge/panel drivers also use two ports
for the connection, so to use the dual link you need two ports in the DSS.
I admit I'm not familiar with LVDS dual link, but it's not clear to me
how you see the dual OLDI TX being used with other drivers if you have
only one port. What kind of setups have you tested?
> The use of lvds helper functions does not seem feasible in this case,
> because even they read DT properties to determine the dual link
> connection and those properties need to be a part of a lvds bridge
> device.
Can you elaborate a bit more why the DRM helpers couldn't be used here?
> I have also been considering the idea of implementing a new device
> driver for the OLDI TXes, not unlike the renesas' one. That way the
> driver could have the properties and the lvds helper functions at their
> disposal. I am just slightly unsure if that would allow space for any
> conflicts because of the shared register space.
No, I don't think new devices are needed here.
Tomi
Hi Tomi,
On 09-Aug-22 11:58, Tomi Valkeinen wrote:
> Hi,
>
> On 09/08/2022 08:58, Aradhya Bhatia wrote:
>> Hi Tomi,
>>
>> On 28-Jul-22 17:15, Tomi Valkeinen wrote:
>>> On 28/07/2022 14:03, Tomi Valkeinen wrote:
>>>> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>>>>> The 2 OLDI TXes in the AM625 SoC can be synced together to output a 2K
>>>>> resolution video.
>>>>>
>>>>> Add support in the driver for the discovery of such a dual mode
>>>>> connection on the OLDI video port, using the values of "ti,oldi-mode"
>>>>> property.
>>>>>
>>>>> Signed-off-by: Aradhya Bhatia <[email protected]>
>>>>> ---
>>>>> drivers/gpu/drm/tidss/tidss_dispc.c | 39
>>>>> +++++++++++++++++++++--------
>>>>> 1 file changed, 28 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>>> index add725fa682b..fb1fdecfc83a 100644
>>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>>> @@ -853,25 +853,36 @@ void dispc_set_irqenable(struct dispc_device
>>>>> *dispc, dispc_irq_t mask)
>>>>> }
>>>>> }
>>>>> -enum dispc_oldi_mode_reg_val { SPWG_18 = 0, JEIDA_24 = 1, SPWG_24
>>>>> = 2 };
>>>>> +enum dispc_oldi_mode_reg_val {
>>>>> + SPWG_18 = 0,
>>>>> + JEIDA_24 = 1,
>>>>> + SPWG_24 = 2,
>>>>> + DL_SPWG_18 = 4,
>>>>> + DL_JEIDA_24 = 5,
>>>>> + DL_SPWG_24 = 6,
>>>>> +};
>>>>> struct dispc_bus_format {
>>>>> u32 bus_fmt;
>>>>> u32 data_width;
>>>>> bool is_oldi_fmt;
>>>>> + bool is_dual_link;
>>>>> enum dispc_oldi_mode_reg_val oldi_mode_reg_val;
>>>>> };
>>>>> static const struct dispc_bus_format dispc_bus_formats[] = {
>>>>> - { MEDIA_BUS_FMT_RGB444_1X12, 12, false, 0 },
>>>>> - { MEDIA_BUS_FMT_RGB565_1X16, 16, false, 0 },
>>>>> - { MEDIA_BUS_FMT_RGB666_1X18, 18, false, 0 },
>>>>> - { MEDIA_BUS_FMT_RGB888_1X24, 24, false, 0 },
>>>>> - { MEDIA_BUS_FMT_RGB101010_1X30, 30, false, 0 },
>>>>> - { MEDIA_BUS_FMT_RGB121212_1X36, 36, false, 0 },
>>>>> - { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, SPWG_18 },
>>>>> - { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, SPWG_24 },
>>>>> - { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, JEIDA_24 },
>>>>> + { MEDIA_BUS_FMT_RGB444_1X12, 12, false, false, 0 },
>>>>> + { MEDIA_BUS_FMT_RGB565_1X16, 16, false, false, 0 },
>>>>> + { MEDIA_BUS_FMT_RGB666_1X18, 18, false, false, 0 },
>>>>> + { MEDIA_BUS_FMT_RGB888_1X24, 24, false, false, 0 },
>>>>> + { MEDIA_BUS_FMT_RGB101010_1X30, 30, false, false, 0 },
>>>>> + { MEDIA_BUS_FMT_RGB121212_1X36, 36, false, false, 0 },
>>>>> + { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, false, SPWG_18 },
>>>>> + { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, false, SPWG_24 },
>>>>> + { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, false,
>>>>> JEIDA_24 },
>>>>> + { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, 18, true, true,
>>>>> DL_SPWG_18 },
>>>>> + { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, 24, true, true,
>>>>> DL_SPWG_24 },
>>>>> + { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, 24, true, true,
>>>>> DL_JEIDA_24 },
>>>>> };
>>>>
>>>> So the dual link sends two pixels per clock, right? Are there panel
>>>> or bridge drivers that support this? My initial thought was that it
>>>> should be a new bus format.
>>>
>>> Looks like we have drm bridges supporting dual link, and they use the
>>> "normal" bus format. Did you have a look at them? They require two
>>> port nodes for dual link, and use the existence of the second one to
>>> decide if dual link is used or not.
>> The above edits were not for adding a new bus format for dual link
>> connections. I added them in order to be able to write the correct OLDI
>> config values in the register.
>>
>>>
>>> There are also lvds helpers in drm_of.c. I didn't look closely, but
>>> it looked to me that the helpers can tell you if the ports are
>>> connected to a dual link bridge. If not, you could fall back to
>>> cloning. This way no extra properties are needed. But you will need
>>> to add a port node, which I think you need to add anyway for cloning.
>> I have now seen drm_of.c and examples (renesas' rcar lvds) that use the
>> apis that drm_of.c is offering. In those cases, the OLDI TXes are being
>> modeled as separate devices, which is not the case with the tidss' OLDI
>> TXes. Since the only few OLDI registers are in the DSS address space,
>> they were just being configured through the tidss driver.
>
> I think it's irrelevant (in the bigger picture) whether the TXes are
> separate devices, single device or part of some other device. Or why do
> you think it matters?
>
Sorry, by separate I meant having a separate driver irrespective of
whether or not its a part of another device.
>> Even in DT, the dss port (for OLDI) connects to the panel port's
>> endpoint directly. Even in cases of dual link or cloning, it's only a
>> singular remote-to-endpoint connection between the (OLDI) VP and the
>> panel port. Hence the requirement of the properties in the earlier
>> patches of the series.
>
> Sorry, I don't follow. If you use cloning, you have two TX outputs,
> going to two panels, right? So you need two panel DT nodes, and those
> would connect to two OLDI TX ports in the DSS.
> > Afaics the existing dual link bridge/panel drivers also use two ports
> for the connection, so to use the dual link you need two ports in the DSS.
>
> I admit I'm not familiar with LVDS dual link, but it's not clear to me
> how you see the dual OLDI TX being used with other drivers if you have
> only one port. What kind of setups have you tested?
>
In the DTs, the OLDIs are not modeled at all. Since the DSS only has a
single VP for OLDI, the DT dss port (for OLDI) is connected to a single
simple-panel node for dual link, bypassing the OLDI TX in DT. I have
this same OLDI setup and have been testing on this.
I do not have a cloning display setup with me, but I have seen DT DSS
port connected to one of 2 panel nodes while the other panel (remains as
a companion panel to the first) without any endpoint connections. Since,
the OLDI TXes (0 and 1), receive the same clocks and inputs from DSS
OLDI VP, this 'method' has worked too.
>> The use of lvds helper functions does not seem feasible in this case,
>> because even they read DT properties to determine the dual link
>> connection and those properties need to be a part of a lvds bridge
>> device.
>
> Can you elaborate a bit more why the DRM helpers couldn't be used here?
>
The drm_of.c helpers use DT properties to ascertain the presence of a
dual-link connection. While there wasn't a specific helper to determine
dual-link or not, the drivers use the odd/even pixel order helper which
is based on the properties "dual-lvds-odd-pixels" and "dual-lvds-odd-
pixels". If either of the properties are absent, the helper returns an
error making the driver to use single link.
These properties are LVDS specific, but they could not be added in the
DT because there is no OLDI TX DT node for our case.
>> I have also been considering the idea of implementing a new device
>> driver for the OLDI TXes, not unlike the renesas' one. That way the
>> driver could have the properties and the lvds helper functions at their
>> disposal. I am just slightly unsure if that would allow space for any
>> conflicts because of the shared register space.
>
> No, I don't think new devices are needed here.
Okay...
I am not quite sure I understand completely what you are recommending
the OLDI to be. It seems to me that you want the OLDI TXes to be modeled
as nodes, right? Wouldn't that automatically require some sort of
standalone driver arrangement for them? Or am I missing something
important here?
Regards
Aradhya
On 09/08/2022 12:06, Aradhya Bhatia wrote:
>>> Even in DT, the dss port (for OLDI) connects to the panel port's
>>> endpoint directly. Even in cases of dual link or cloning, it's only a
>>> singular remote-to-endpoint connection between the (OLDI) VP and the
>>> panel port. Hence the requirement of the properties in the earlier
>>> patches of the series.
>>
>> Sorry, I don't follow. If you use cloning, you have two TX outputs,
>> going to two panels, right? So you need two panel DT nodes, and those
>> would connect to two OLDI TX ports in the DSS.
>> > Afaics the existing dual link bridge/panel drivers also use two ports
>> for the connection, so to use the dual link you need two ports in the
>> DSS.
>>
>> I admit I'm not familiar with LVDS dual link, but it's not clear to me
>> how you see the dual OLDI TX being used with other drivers if you have
>> only one port. What kind of setups have you tested?
>>
> In the DTs, the OLDIs are not modeled at all. Since the DSS only has a
> single VP for OLDI, the DT dss port (for OLDI) is connected to a single
> simple-panel node for dual link, bypassing the OLDI TX in DT. I have
> this same OLDI setup and have been testing on this.
A DSS VP is a DSS internal port, whereas a port node in the DT is an
external port. There doesn't have to be a 1:1 match between those.
The port in the DT represents some kind of "connector" to the outside
world, which is usually a collection of pins that provide a video bus.
Here, as far as I can see, the DSS clearly has three external ports, two
OLDI ports and one DPI port.
> I do not have a cloning display setup with me, but I have seen DT DSS
> port connected to one of 2 panel nodes while the other panel (remains as
> a companion panel to the first) without any endpoint connections. Since,
> the OLDI TXes (0 and 1), receive the same clocks and inputs from DSS
> OLDI VP, this 'method' has worked too.
This, and using simple-panel for dual link with single port connection,
sounds like a hack.
A practical example: TI's customer wants to use AM625 and THC63LVD1024
bridge. How does it work? THC63LVD1024 driver uses two LVDS ports for
input, both of which are used in dual-link mode.
>>> The use of lvds helper functions does not seem feasible in this case,
>>> because even they read DT properties to determine the dual link
>>> connection and those properties need to be a part of a lvds bridge
>>> device.
>>
>> Can you elaborate a bit more why the DRM helpers couldn't be used here?
>>
> The drm_of.c helpers use DT properties to ascertain the presence of a
> dual-link connection. While there wasn't a specific helper to determine
> dual-link or not, the drivers use the odd/even pixel order helper which
> is based on the properties "dual-lvds-odd-pixels" and "dual-lvds-odd-
> pixels". If either of the properties are absent, the helper returns an
> error making the driver to use single link.
>
> These properties are LVDS specific, but they could not be added in the
> DT because there is no OLDI TX DT node for our case.
If I'm not mistaken, those properties are in the port node, not the
device node, and also, I believe those properties are on the sink side,
so they wouldn't even be in the AM625 data. See, for example:
arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts
>>> I have also been considering the idea of implementing a new device
>>> driver for the OLDI TXes, not unlike the renesas' one. That way the
>>> driver could have the properties and the lvds helper functions at their
>>> disposal. I am just slightly unsure if that would allow space for any
>>> conflicts because of the shared register space.
>>
>> No, I don't think new devices are needed here.
> Okay...
>
> I am not quite sure I understand completely what you are recommending
> the OLDI to be. It seems to me that you want the OLDI TXes to be modeled
> as nodes, right? Wouldn't that automatically require some sort of
> standalone driver arrangement for them? Or am I missing something
> important here?
No, I'm only talking about the DT port nodes. At the moment the AM65x DT
bindings doc says that there are two ports, port@0 for OLDI and port@1
for DPI. I'm saying AM625 needs three ports.
Tomi
Hi Tomi,
On 09-Aug-22 15:21, Tomi Valkeinen wrote:
> On 09/08/2022 12:06, Aradhya Bhatia wrote:
>
>>>> Even in DT, the dss port (for OLDI) connects to the panel port's
>>>> endpoint directly. Even in cases of dual link or cloning, it's only a
>>>> singular remote-to-endpoint connection between the (OLDI) VP and the
>>>> panel port. Hence the requirement of the properties in the earlier
>>>> patches of the series.
>>>
>>> Sorry, I don't follow. If you use cloning, you have two TX outputs,
>>> going to two panels, right? So you need two panel DT nodes, and those
>>> would connect to two OLDI TX ports in the DSS.
>>> > Afaics the existing dual link bridge/panel drivers also use two ports
>>> for the connection, so to use the dual link you need two ports in the
>>> DSS.
>>>
>>> I admit I'm not familiar with LVDS dual link, but it's not clear to
>>> me how you see the dual OLDI TX being used with other drivers if you
>>> have only one port. What kind of setups have you tested?
>>>
>> In the DTs, the OLDIs are not modeled at all. Since the DSS only has a
>> single VP for OLDI, the DT dss port (for OLDI) is connected to a single
>> simple-panel node for dual link, bypassing the OLDI TX in DT. I have
>> this same OLDI setup and have been testing on this.
>
> A DSS VP is a DSS internal port, whereas a port node in the DT is an
> external port. There doesn't have to be a 1:1 match between those.
>
> The port in the DT represents some kind of "connector" to the outside
> world, which is usually a collection of pins that provide a video bus.
>
Okay, I now understand what you are saying. Indeed, I was mapping the
DSS VP and DT DSS port as 1:1 in my mind, which should not be the case.
> Here, as far as I can see, the DSS clearly has three external ports, two
> OLDI ports and one DPI port.
>
>> I do not have a cloning display setup with me, but I have seen DT DSS
>> port connected to one of 2 panel nodes while the other panel (remains as
>> a companion panel to the first) without any endpoint connections. Since,
>> the OLDI TXes (0 and 1), receive the same clocks and inputs from DSS
>> OLDI VP, this 'method' has worked too.
>
> This, and using simple-panel for dual link with single port connection,
> sounds like a hack.
>
> A practical example: TI's customer wants to use AM625 and THC63LVD1024
> bridge. How does it work? THC63LVD1024 driver uses two LVDS ports for
> input, both of which are used in dual-link mode.
>
Right. There has to be 2 ports for OLDI in DSS, to be connected to 2
ports of a single panel (dual link) or 2 ports of 2 panels (cloning).
>>>> The use of lvds helper functions does not seem feasible in this case,
>>>> because even they read DT properties to determine the dual link
>>>> connection and those properties need to be a part of a lvds bridge
>>>> device.
>>>
>>> Can you elaborate a bit more why the DRM helpers couldn't be used here?
>>>
>> The drm_of.c helpers use DT properties to ascertain the presence of a
>> dual-link connection. While there wasn't a specific helper to determine
>> dual-link or not, the drivers use the odd/even pixel order helper which
>> is based on the properties "dual-lvds-odd-pixels" and "dual-lvds-odd-
>> pixels". If either of the properties are absent, the helper returns an
>> error making the driver to use single link.
>>
>> These properties are LVDS specific, but they could not be added in the
>> DT because there is no OLDI TX DT node for our case.
>
> If I'm not mistaken, those properties are in the port node, not the
> device node, and also, I believe those properties are on the sink side,
> so they wouldn't even be in the AM625 data. See, for example:
>
> arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts
Yeah, they are indeed on the sink side. I was misunderstood about this.
And the onus for properties is not on DSS nodes anymore.
This probably is a different discussion, but since the sink is now
responsible for those properties, these should get introduced in the
panel-common bindings, right?
The above example has a separate binding, but many dumb dual-link panels
will require those properties in panel-common.
>
>>>> I have also been considering the idea of implementing a new device
>>>> driver for the OLDI TXes, not unlike the renesas' one. That way the
>>>> driver could have the properties and the lvds helper functions at their
>>>> disposal. I am just slightly unsure if that would allow space for any
>>>> conflicts because of the shared register space.
>>>
>>> No, I don't think new devices are needed here.
>> Okay...
>>
>> I am not quite sure I understand completely what you are recommending
>> the OLDI to be. It seems to me that you want the OLDI TXes to be modeled
>> as nodes, right? Wouldn't that automatically require some sort of
>> standalone driver arrangement for them? Or am I missing something
>> important here?
>
> No, I'm only talking about the DT port nodes. At the moment the AM65x DT
> bindings doc says that there are two ports, port@0 for OLDI and port@1
> for DPI. I'm saying AM625 needs three ports.
Agreed.
Moreover, I will update the binding to reflect 3 ports for am625-dss.
Regards
Aradhya