Initial support Cadence MHDP8501(HDMI/DP) for i.MX8MQ

MHDP8546 mailbox access functions will be share to other mhdp driver and Cadence HDP-TX HDMI/DP PHY drivers.
Create a new mhdp helper driver and move all those functions into.

cdns_mhdp_reg_write() is renamed to cdns_mhdp_dp_reg_write(), because it use the DPTX command ID DPTX_WRITE_REGISTER.

New cdns_mhdp_reg_write() is created with the general command ID GENERAL_REGISTER_WRITE.

Rewrite cdns_mhdp_set_firmware_active() in mhdp8546 core driver, use cdns_mhdp_mailbox_send() to replace cdns_mhdp_mailbox_write() same as the other mailbox access functions.

Replaces the local mutex mbox_mutex with a global mutex mhdp_mailbox_mutex to prevent race conditions in mailbox access by multi drivers.

Signed-off-by Sandor Yu <[email protected]>

---
v16->v17:
- Replaces the local mutex mbox_mutex with a global mutex mhdp_mailbox_mutex

v12->v16:
*No change.

V11->v12:
- Move status initialize out of mbox_mutex.
- Reorder API functions in alphabetical.
- Add notes for malibox access functions.
- Add year 2024 to copyright.

 drivers/gpu/drm/bridge/cadence/Kconfig        |   4 +
 drivers/gpu/drm/bridge/cadence/Makefile       |   1 +
 .../gpu/drm/bridge/cadence/cdns-mhdp-helper.c | 307 +++++++++++++
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 425 ++++--------------
 .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  47 +-
 .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   |  36 +-
 include/drm/bridge/cdns-mhdp-helper.h         |  94 ++++
 7 files changed, 507 insertions(+), 407 deletions(-)
 create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
 create mode 100644 include/drm/bridge/cdns-mhdp-helper.h

drivers/gpu/drm/bridge/cadence/Kconfig +4 -0

index cced81633ddcd..e0973339e9e33 100644
@@ -21,6 +21,9 @@ config DRM_CDNS_DSI_J721E
 	  the routing of the DSS DPI signal to the Cadence DSI.
 endif
 
+config CDNS_MHDP_HELPER
+	tristate
+
 config DRM_CDNS_MHDP8546
 	tristate "Cadence DPI/DP bridge"
 	select DRM_DISPLAY_DP_HELPER

@@ -28,6 +31,7 @@ config DRM_CDNS_MHDP8546
 	select DRM_DISPLAY_HELPER
 	select DRM_KMS_HELPER
 	select DRM_PANEL_BRIDGE
+	select CDNS_MHDP_HELPER
 	depends on OF
 	help
 	  Support Cadence DPI to DP bridge. This is an internal

drivers/gpu/drm/bridge/cadence/Makefile +1 -0

index c95fd5b81d137..087dc074820d7 100644
@@ -2,6 +2,7 @@
 obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
 cdns-dsi-y := cdns-dsi-core.o
 cdns-dsi-$(CONFIG_DRM_CDNS_DSI_J721E) += cdns-dsi-j721e.o
+obj-$(CONFIG_CDNS_MHDP_HELPER) += cdns-mhdp-helper.o
 obj-$(CONFIG_DRM_CDNS_MHDP8546) += cdns-mhdp8546.o
 cdns-mhdp8546-y := cdns-mhdp8546-core.o cdns-mhdp8546-hdcp.o
 cdns-mhdp8546-$(CONFIG_DRM_CDNS_MHDP8546_J721E) += cdns-mhdp8546-j721e.o

drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c new file +1 -0

new file mode 100644
index 0000000000000..c60a6b69a5343
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0-only

ERROR: modpost: "mhdp_mailbox_mutex" [drivers/gpu/drm/bridge/cadence/cdns-mhdp8546.ko] undefined! ERROR:
modpost: "mhdp_mailbox_mutex" [drivers/gpu/drm/bridge/cadence/cdns-mhdp8501.ko] undefined!

[snip]

Hi Sandor,

Am Dienstag, 24. September 2024, 09:36:46 CEST schrieb Sandor Yu:

Quoted 83 lines

MHDP8546 mailbox access functions will be share to other mhdp driver and Cadence HDP-TX HDMI/DP PHY drivers.
Create a new mhdp helper driver and move all those functions into.

cdns_mhdp_reg_write() is renamed to cdns_mhdp_dp_reg_write(), because it use the DPTX command ID DPTX_WRITE_REGISTER.

New cdns_mhdp_reg_write() is created with the general command ID GENERAL_REGISTER_WRITE.

Rewrite cdns_mhdp_set_firmware_active() in mhdp8546 core driver, use cdns_mhdp_mailbox_send() to replace cdns_mhdp_mailbox_write() same as the other mailbox access functions.

Replaces the local mutex mbox_mutex with a global mutex mhdp_mailbox_mutex to prevent race conditions in mailbox access by multi drivers.

Signed-off-by Sandor Yu <[email protected]>

---
v16->v17:
- Replaces the local mutex mbox_mutex with a global mutex mhdp_mailbox_mutex

v12->v16:
*No change.

V11->v12:
- Move status initialize out of mbox_mutex.
- Reorder API functions in alphabetical.
- Add notes for malibox access functions.
- Add year 2024 to copyright.

 drivers/gpu/drm/bridge/cadence/Kconfig        |   4 +
 drivers/gpu/drm/bridge/cadence/Makefile       |   1 +
 .../gpu/drm/bridge/cadence/cdns-mhdp-helper.c | 307 +++++++++++++
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 425 ++++--------------
 .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  47 +-
 .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   |  36 +-
 include/drm/bridge/cdns-mhdp-helper.h         |  94 ++++
 7 files changed, 507 insertions(+), 407 deletions(-)
 create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
 create mode 100644 include/drm/bridge/cdns-mhdp-helper.h

drivers/gpu/drm/bridge/cadence/Kconfig +4 -0

index cced81633ddcd..e0973339e9e33 100644
@@ -21,6 +21,9 @@ config DRM_CDNS_DSI_J721E
 	  the routing of the DSS DPI signal to the Cadence DSI.
 endif
 
+config CDNS_MHDP_HELPER
+	tristate
+
 config DRM_CDNS_MHDP8546
 	tristate "Cadence DPI/DP bridge"
 	select DRM_DISPLAY_DP_HELPER

@@ -28,6 +31,7 @@ config DRM_CDNS_MHDP8546
 	select DRM_DISPLAY_HELPER
 	select DRM_KMS_HELPER
 	select DRM_PANEL_BRIDGE
+	select CDNS_MHDP_HELPER
 	depends on OF
 	help
 	  Support Cadence DPI to DP bridge. This is an internal

drivers/gpu/drm/bridge/cadence/Makefile +1 -0

index c95fd5b81d137..087dc074820d7 100644
@@ -2,6 +2,7 @@
 obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
 cdns-dsi-y := cdns-dsi-core.o
 cdns-dsi-$(CONFIG_DRM_CDNS_DSI_J721E) += cdns-dsi-j721e.o
+obj-$(CONFIG_CDNS_MHDP_HELPER) += cdns-mhdp-helper.o
 obj-$(CONFIG_DRM_CDNS_MHDP8546) += cdns-mhdp8546.o
 cdns-mhdp8546-y := cdns-mhdp8546-core.o cdns-mhdp8546-hdcp.o
 cdns-mhdp8546-$(CONFIG_DRM_CDNS_MHDP8546_J721E) += cdns-mhdp8546-j721e.o

drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c new file +1 -0

new file mode 100644
index 0000000000000..c60a6b69a5343
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0-only

drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c line 11

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c new file +10 -0

+/*
+ * Copyright (C) 2023, 2024 NXP Semiconductor, Inc.
+ *
+ */
+#include <drm/bridge/cdns-mhdp-helper.h>
+#include <linux/dev_printk.h>
+#include <linux/module.h>
+
+/* Protects mailbox communications with the firmware */
+DEFINE_MUTEX(mhdp_mailbox_mutex);

This is not enough if the driver is built as a module:

Quoted 4 lines

ERROR: modpost: "mhdp_mailbox_mutex" [drivers/gpu/drm/bridge/cadence/cdns-mhdp8546.ko] undefined! ERROR:
modpost: "mhdp_mailbox_mutex" [drivers/gpu/drm/bridge/cadence/cdns-mhdp8501.ko] undefined!

Not sure if EXPORT_SYMBOL_GPL() on a mutex is considered good style.

Best regards,
Alexander

Quoted 1 line

[snip]

Signature · 4 lines

TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Allow HDMI PHYs to be configured through the generic functions through a custom structure added to the generic union.

The parameters added here are based on HDMI PHY implementation practices. The current set of parameters

[ ... ]

Add bindings for Cadence MHDP8501 DisplayPort/HDMI bridge.

Signed-off-by Sandor Yu <[email protected]>

Reviewed-by Krzysztof Kozlowski <[email protected]>

---
v16->v17:
- Add lane-mapping property

v9->v16:
*No change

.../display/bridge/cdns,mhdp8501.yaml | 109 ++++++++++++++++++

 1 file changed, 109 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml new file +33 -0

new file mode 100644
index 0000000000000..3f79f328c7425
@@ -0,0 +1,109 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/cdns,mhdp8501.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cadence MHDP8501 DP/HDMI bridge
+
+maintainers:
+  - Sandor Yu <[email protected]>
+
+description:
+  Cadence MHDP8501 DisplayPort/HDMI interface.
+
+properties:
+  compatible:
+    enum:
+      - fsl,imx8mq-mhdp8501
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: MHDP8501 DP/HDMI APB clock.
+
+  phys:
+    maxItems: 1
+    description:
+      phandle to the DP/HDMI PHY
+
+  interrupts:
+    items:

Quoted 4 lines

Add bindings for Cadence MHDP8501 DisplayPort/HDMI bridge.

Signed-off-by Sandor Yu <[email protected]>

Reviewed-by Krzysztof Kozlowski <[email protected]>

Drop

Quoted 4 lines

---
v16->v17:
- Add lane-mapping property

That's a significant change.

Quoted 6 lines

v9->v16:
*No change

.../display/bridge/cdns,mhdp8501.yaml | 109

++++++++++++++++++

Quoted 15 lines

 1 file changed, 109 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml new file mode 100644
index 0000000000000..3f79f328c7425

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y new file +1 -0

+++ aml

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y line 4

Quoted 4 lines

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y new file +4 -0

@@ -0,0 +1,109 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
+---
+$id:
+https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi

+cetree.org%2Fschemas%2Fdisplay%2Fbridge%2Fcdns%2Cmhdp8501.yaml%2 3&dat

Quoted 1 line

+a=05%7C02%7CSandor.yu%40nxp.com%7C40a6bd4ff1cd4d934da008dcdc72 9fd0%7C

Quoted 1 line

+686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63862763207998660 8%7CUnkno

Quoted 1 line

+wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h aWwi

Quoted 1 line

+LCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Uw%2FQQ0Qg36Y8Q6wFPC7Zg LzLHvOj8GjH1

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y

Quoted 4 lines

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y new file +3 -0

+k8McgcjrqI%3D&reserved=0
+$schema:
+https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi

+cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C02%7CSandor.y u%40n

Quoted 1 line

+xp.com%7C40a6bd4ff1cd4d934da008dcdc729fd0%7C686ea1d3bc2b4c6fa92 cd99c5

Quoted 1 line

+c301635%7C0%7C0%7C638627632080031124%7CUnknown%7CTWFpbGZs b3d8eyJWIjoi

Quoted 1 line

+MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0% 7C%7C%

Quoted 1 line

+7C&sdata=IG9Em3AWOwzlpR1Wk0Vw%2FF161YcOxuMHbA4Y%2FzftgYA%3D &reserved=

Quoted 34 lines

+0

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y new file +33 -0

+
+title: Cadence MHDP8501 DP/HDMI bridge
+
+maintainers:
+  - Sandor Yu <[email protected]>
+
+description:
+  Cadence MHDP8501 DisplayPort/HDMI interface.
+
+properties:
+  compatible:
+    enum:
+      - fsl,imx8mq-mhdp8501
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: MHDP8501 DP/HDMI APB clock.
+
+  phys:
+    maxItems: 1
+    description:
+      phandle to the DP/HDMI PHY
+
+  interrupts:
+    items:
+      - description: Hotplug cable plugin.
+      - description: Hotplug cable plugout.
+
+  interrupt-names:
+    items:

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y

Quoted 5 lines

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y new file +5 -0

+      - const: plug_in
+      - const: plug_out
+
+  lane-mapping:
+    description: lane mapping for HDMI or DisplayPort interface.

Where is the definition of this property? I do not see any $ref here, so did you add it to dtschema?

Best regards,
Krzysztof

Hi Krzysztof，

Thanks for your comments,

Quoted 9 lines · Sandor Yu

Quoted 4 lines

Add bindings for Cadence MHDP8501 DisplayPort/HDMI bridge.

Signed-off-by Sandor Yu <[email protected]>

Reviewed-by Krzysztof Kozlowski <[email protected]>

Drop

OK, I will remove it in the next version.

Quoted 6 lines

Quoted 4 lines

---
v16->v17:
- Add lane-mapping property

That's a significant change.

OK.

Quoted 92 lines

Quoted 6 lines

v9->v16:
*No change

.../display/bridge/cdns,mhdp8501.yaml | 109

++++++++++++++++++

Quoted 15 lines

 1 file changed, 109 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml new file mode 100644
index 0000000000000..3f79f328c7425

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y new file +1 -0

+++ aml

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y line 4

Quoted 4 lines

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y new file +4 -0

@@ -0,0 +1,109 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
+---
+$id:
+https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi

+cetree.org%2Fschemas%2Fdisplay%2Fbridge%2Fcdns%2Cmhdp8501.yaml%2 3&dat

Quoted 1 line

+a=05%7C02%7CSandor.yu%40nxp.com%7C40a6bd4ff1cd4d934da008dcdc72 9fd0%7C

Quoted 1 line

+686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63862763207998660 8%7CUnkno

Quoted 1 line

+wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h aWwi

Quoted 1 line

+LCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Uw%2FQQ0Qg36Y8Q6wFPC7Zg LzLHvOj8GjH1

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y

Quoted 4 lines

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y new file +3 -0

+k8McgcjrqI%3D&reserved=0
+$schema:
+https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi

+cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C02%7CSandor.y u%40n

Quoted 1 line

+xp.com%7C40a6bd4ff1cd4d934da008dcdc729fd0%7C686ea1d3bc2b4c6fa92 cd99c5

Quoted 1 line

+c301635%7C0%7C0%7C638627632080031124%7CUnknown%7CTWFpbGZs b3d8eyJWIjoi

Quoted 1 line

+MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0% 7C%7C%

Quoted 1 line

+7C&sdata=IG9Em3AWOwzlpR1Wk0Vw%2FF161YcOxuMHbA4Y%2FzftgYA%3D &reserved=

Quoted 34 lines

+0

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y new file +33 -0

+
+title: Cadence MHDP8501 DP/HDMI bridge
+
+maintainers:
+  - Sandor Yu <[email protected]>
+
+description:
+  Cadence MHDP8501 DisplayPort/HDMI interface.
+
+properties:
+  compatible:
+    enum:
+      - fsl,imx8mq-mhdp8501
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: MHDP8501 DP/HDMI APB clock.
+
+  phys:
+    maxItems: 1
+    description:
+      phandle to the DP/HDMI PHY
+
+  interrupts:
+    items:
+      - description: Hotplug cable plugin.
+      - description: Hotplug cable plugout.
+
+  interrupt-names:
+    items:

Quoted 8 lines

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y

Quoted 5 lines

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y new file +5 -0

+      - const: plug_in
+      - const: plug_out
+
+  lane-mapping:
+    description: lane mapping for HDMI or DisplayPort interface.

Where is the definition of this property? I do not see any $ref here, so did you add it to dtschema?

My apologies, the $ref is missing, will add it in the next version..

Quoted 16 lines

Hi Krzysztof，

Thanks for your comments,

Quoted 9 lines · Sandor Yu

Quoted 4 lines

Add bindings for Cadence MHDP8501 DisplayPort/HDMI bridge.

Signed-off-by Sandor Yu <[email protected]>

Reviewed-by Krzysztof Kozlowski <[email protected]>

Drop

OK, I will remove it in the next version.

Quoted 1 line

Quoted 20 lines

Quoted 5 lines

Quoted 3 lines

---
v16->v17:
- Add lane-mapping property

That's a significant change.

OK.

Quoted 13 lines

Quoted 6 lines

v9->v16:
*No change

.../display/bridge/cdns,mhdp8501.yaml | 109

++++++++++++++++++

Quoted 6 lines

 1 file changed, 109 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml

diff --git

a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yam

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

l

b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yam

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

l
new file mode 100644
index 0000000000000..3f79f328c7425
--- /dev/null
+++

b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501

Quoted 8 lines

Quoted 8 lines

Review line 3

Quoted 8 lines

+++ .y
+++ aml

(quoted diff) +3 -0

@@ -0,0 +1,109 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
+---
+$id:

+https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde

Quoted 1 line

Quoted 1 line

Quoted 1 line

+vi%2F&data=05%7C02%7Csandor.yu%40nxp.com%7C3c6bfe07c23945a1052 608dc

Quoted 1 line

Quoted 1 line

Quoted 1 line

+e13ffde7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6386329 121918

Quoted 1 line

Quoted 1 line

Quoted 1 line

+65139%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2 luMzIiL

Quoted 1 line

Quoted 1 line

Quoted 1 line

+CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=yrG%2FJsGK5 czDeRwf

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

+A8e6%2F5r0Ui5v95uiC4OlK7DJ5QY%3D&reserved=0

+cetree.org%2Fschemas%2Fdisplay%2Fbridge%2Fcdns%2Cmhdp8501.yaml% 2

Quoted 3 lines

Quoted 3 lines

3&dat

Quoted 1 line

+a=05%7C02%7CSandor.yu%40nxp.com%7C40a6bd4ff1cd4d934da008dcdc72

Quoted 3 lines

Quoted 3 lines

9fd0%7C

Quoted 1 line

+686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63862763207998660

Quoted 3 lines

Quoted 3 lines

8%7CUnkno

Quoted 1 line

+wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1 h

Quoted 3 lines

Quoted 3 lines

aWwi

Quoted 1 line

+LCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Uw%2FQQ0Qg36Y8Q6wFPC7Z g

Quoted 4 lines

Quoted 4 lines

LzLHvOj8GjH1

Quoted 3 lines

+k8McgcjrqI%3D&reserved=0
+$schema:

+https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde

Quoted 1 line

Quoted 1 line

Quoted 1 line

+vi%2F&data=05%7C02%7Csandor.yu%40nxp.com%7C3c6bfe07c23945a1052 608dc

Quoted 1 line

Quoted 1 line

Quoted 1 line

+e13ffde7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6386329 121918

Quoted 1 line

Quoted 1 line

Quoted 1 line

+95630%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2 luMzIiL

Quoted 1 line

Quoted 1 line

Quoted 1 line

+CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=%2BKccVMx nf5oUwZgf

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

+smPXiKN2VycnLCdDKx6FZFBBAJM%3D&reserved=0

+cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C02%7CSandor. y

Quoted 3 lines

Quoted 3 lines

u%40n

Quoted 1 line

+xp.com%7C40a6bd4ff1cd4d934da008dcdc729fd0%7C686ea1d3bc2b4c6fa9 2

Quoted 3 lines

Quoted 3 lines

cd99c5

Quoted 1 line

+c301635%7C0%7C0%7C638627632080031124%7CUnknown%7CTWFpbGZs

Quoted 3 lines

Quoted 3 lines

b3d8eyJWIjoi

Quoted 1 line

+MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0 %

Quoted 3 lines

Quoted 3 lines

7C%7C%

Quoted 1 line

+7C&sdata=IG9Em3AWOwzlpR1Wk0Vw%2FF161YcOxuMHbA4Y%2FzftgYA%3 D

Quoted 44 lines

Quoted 43 lines

&reserved=

Quoted 29 lines

+0

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml new file +28 -0

+
+title: Cadence MHDP8501 DP/HDMI bridge
+
+maintainers:
+  - Sandor Yu <[email protected]>
+
+description:
+  Cadence MHDP8501 DisplayPort/HDMI interface.
+
+properties:
+  compatible:
+    enum:
+      - fsl,imx8mq-mhdp8501
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: MHDP8501 DP/HDMI APB clock.
+
+  phys:
+    maxItems: 1
+    description:
+      phandle to the DP/HDMI PHY
+
+  interrupts:
+    items:

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml line 43

Quoted 10 lines

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml new file +10 -0

+      - description: Hotplug cable plugin.
+      - description: Hotplug cable plugout.
+
+  interrupt-names:
+    items:
+      - const: plug_in
+      - const: plug_out
+
+  lane-mapping:
+    description: lane mapping for HDMI or DisplayPort interface.

Where is the definition of this property? I do not see any $ref here, so did you add it to dtschema?

Quoted 1 line

My apologies, the $ref is missing, will add it in the next version..

And that's different than existing properties, e.g. data-lanes or lane-polarities? There is no description here except copying property name, which is not useful at all.

Come with solution matching other bridges and media devices.

Best regards,
Krzysztof

Add bindings for Cadence MHDP8501 DisplayPort/HDMI bridge.

Signed-off-by Sandor Yu <[email protected]>

Reviewed-by Krzysztof Kozlowski <[email protected]>

---
v16->v17:
- Add lane-mapping property

v9->v16:
*No change

.../display/bridge/cdns,mhdp8501.yaml | 109 ++++++++++++++++++

 1 file changed, 109 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml new file +77 -0

new file mode 100644
index 0000000000000..3f79f328c7425
@@ -0,0 +1,109 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/cdns,mhdp8501.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cadence MHDP8501 DP/HDMI bridge
+
+maintainers:
+  - Sandor Yu <[email protected]>
+
+description:
+  Cadence MHDP8501 DisplayPort/HDMI interface.
+
+properties:
+  compatible:
+    enum:
+      - fsl,imx8mq-mhdp8501
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: MHDP8501 DP/HDMI APB clock.
+
+  phys:
+    maxItems: 1
+    description:
+      phandle to the DP/HDMI PHY
+
+  interrupts:
+    items:
+      - description: Hotplug cable plugin.
+      - description: Hotplug cable plugout.
+
+  interrupt-names:
+    items:
+      - const: plug_in
+      - const: plug_out
+
+  lane-mapping:
+    description: lane mapping for HDMI or DisplayPort interface.
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          Input port from display controller output.
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          Output port to DisplayPort or HDMI connector.
+
+    required:
+      - port@0
+      - port@1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - interrupts
+  - interrupt-names
+  - phys
+  - lane-mapping
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx8mq-clock.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>

Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml new file +22 -0

+
+        ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            port@0 {
+                reg = <0>;
+
+                mhdp_in: endpoint {
+                    remote-endpoint = <&dcss_out>;
+                };
+            };
+
+            port@1 {
+                reg = <1>;
+
+                mhdp_out: endpoint {
+                    remote-endpoint = <&dp_connector>;
+                };
+            };
+        };
+    };

Signature · 1 line

2.34.1

Add bindings for Cadence MHDP8501 DisplayPort/HDMI bridge.

Signed-off-by Sandor Yu <[email protected]>

Reviewed-by Krzysztof Kozlowski <[email protected]>

---
v16->v17:
- Add lane-mapping property

v9->v16:
*No change

.../display/bridge/cdns,mhdp8501.yaml | 109 ++++++++++++++++++

 1 file changed, 109 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +13 -0

+static int cdns_mhdp8501_read_hpd(struct cdns_mhdp8501_device *mhdp)
+{
+	u8 status;
+	int ret;
+
+	mutex_lock(&mhdp_mailbox_mutex);
+
+	ret = cdns_mhdp_mailbox_send(&mhdp->base, MB_MODULE_ID_GENERAL,
+				     GENERAL_GET_HPD_STATE, 0, NULL);
+	if (ret)
+		goto err_get_hpd;
+
+	ret = cdns_mhdp_mailbox_recv_header(&mhdp->base, MB_MODULE_ID_GENERAL,

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +10 -0

+	return status;
+
+err_get_hpd:
+	dev_err(mhdp->dev, "read hpd  failed: %d\n", ret);
+	mutex_unlock(&mhdp_mailbox_mutex);
+
+	return ret;
+}
+
+enum drm_connector_status cdns_mhdp8501_detect(struct cdns_mhdp8501_device *mhdp)

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +5 -0

+	return connector_status_unknown;
+}
+
+static void hotplug_work_func(struct work_struct *work)
+{

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +16 -0

+		enable_irq(mhdp->irq[IRQ_OUT]);
+
+		/* Reset HDMI/DP link with sink */
+		if (mhdp->connector_type == DRM_MODE_CONNECTOR_HDMIA)
+			cdns_hdmi_reset_link(mhdp);
+		else
+			cdns_dp_check_link_state(mhdp);
+
+	} else if (status == connector_status_disconnected) {
+		/* Cable Disconnected  */
+		DRM_INFO("HDMI/DP Cable Plug Out\n");
+		enable_irq(mhdp->irq[IRQ_IN]);
+	}
+}
+
+static irqreturn_t cdns_mhdp8501_irq_thread(int irq, void *data)

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +22 -0

+
+static int cdns_mhdp8501_dt_parse(struct cdns_mhdp8501_device *mhdp,
+				  struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *remote;
+
+	remote = of_graph_get_remote_node(np, 1, 0);
+	if (!remote) {
+		dev_err(dev, "fail to get remote node\n");
+		of_node_put(remote);
+		return -EINVAL;
+	}
+
+	/* get connector type */
+	if (of_device_is_compatible(remote, "hdmi-connector")) {
+		mhdp->connector_type = DRM_MODE_CONNECTOR_HDMIA;
+
+	} else if (of_device_is_compatible(remote, "dp-connector")) {
+		mhdp->connector_type = DRM_MODE_CONNECTOR_DisplayPort;
+

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +15 -0

+		mhdp->lane_mapping = LANE_MAPPING_FLIPPED;
+	}
+
+	return true;
+}
+
+static void cdns_mhdp8501_add_bridge(struct cdns_mhdp8501_device *mhdp)
+{
+	mhdp->bridge.type = mhdp->connector_type;
+	mhdp->bridge.driver_private = mhdp;
+	mhdp->bridge.of_node = mhdp->dev->of_node;
+	mhdp->bridge.vendor = "NXP";
+	mhdp->bridge.product = "i.MX8";
+	mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
+			   DRM_BRIDGE_OP_HPD;

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +28 -0

+static int cdns_hdmi_scdc_write(struct cdns_mhdp8501_device *mhdp, u8 addr, u8 value)
+{
+	u8 msg[5], reg[5];
+	int ret;
+
+	msg[0] = 0x54;
+	msg[1] = addr;
+	msg[2] = 0;
+	msg[3] = 1;
+	msg[4] = value;
+
+	mutex_lock(&mhdp_mailbox_mutex);
+
+	ret = cdns_mhdp_mailbox_send(&mhdp->base, MB_MODULE_ID_HDMI_TX, HDMI_TX_WRITE,
+				     sizeof(msg), msg);
+	if (ret)
+		goto err_scdc_write;
+
+	ret = cdns_mhdp_mailbox_recv_header(&mhdp->base, MB_MODULE_ID_HDMI_TX,
+					    HDMI_TX_WRITE, sizeof(reg));
+	if (ret)
+		goto err_scdc_write;
+
+	ret = cdns_mhdp_mailbox_recv_data(&mhdp->base, reg, sizeof(reg));
+	if (ret)
+		goto err_scdc_write;
+
+	if (reg[0] != 0)

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +5 -0

+	struct drm_scdc *scdc = &mhdp->curr_conn->display_info.hdmi.scdc;
+	u8 buff = 0;
+
+	/* check sink type (HDMI or DVI) */
+	if (!display->is_hdmi) {

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +29 -0

+static enum drm_mode_status
+cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+			       const struct drm_display_mode *mode,
+			       unsigned long long tmds_rate)
+{
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	phy_cfg.hdmi.tmds_char_rate = tmds_rate;
+
+	ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0, &phy_cfg);
+	if (ret < 0)
+		return MODE_CLOCK_RANGE;
+
+	return MODE_OK;
+}
+
+static enum drm_mode_status
+cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+			    const struct drm_display_info *info,
+			    const struct drm_display_mode *mode)
+{
+	unsigned long long tmds_rate;
+
+	/* We don't support double-clocked and Interlaced modes */
+	if (mode->flags & DRM_MODE_FLAG_DBLCLK ||
+	    mode->flags & DRM_MODE_FLAG_INTERLACE)
+		return MODE_BAD;

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +53 -0

+static void cdns_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
+					   struct drm_bridge_state *old_state)
+{
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	struct drm_atomic_state *state = old_state->base.state;
+	struct drm_connector *connector;
+	struct video_info *video_info = &mhdp->video_info;
+	struct drm_crtc_state *crtc_state;
+	struct drm_connector_state *conn_state;
+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	connector = drm_atomic_get_new_connector_for_encoder(state,
+							     bridge->encoder);
+	if (WARN_ON(!connector))
+		return;
+
+	mhdp->curr_conn = connector;
+
+	conn_state = drm_atomic_get_new_connector_state(state, connector);
+	if (WARN_ON(!conn_state))
+		return;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
+	if (WARN_ON(!crtc_state))
+		return;
+
+	video_info->color_fmt = conn_state->hdmi.output_format;
+	video_info->bpc = conn_state->hdmi.output_bpc;
+
+	drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
+
+	/* Line swapping */
+	cdns_mhdp_reg_write(&mhdp->base, LANES_CONFIG, 0x00400000 | mhdp->lane_mapping);
+
+	phy_cfg.hdmi.tmds_char_rate = conn_state->hdmi.tmds_char_rate;
+
+	ret = phy_configure(mhdp->phy, &phy_cfg);
+	if (ret) {
+		dev_err(mhdp->dev, "%s: phy_configure() failed: %d\n",
+			__func__, ret);
+		return;
+	}
+
+	cdns_hdmi_sink_config(mhdp);
+
+	ret = cdns_hdmi_ctrl_init(mhdp);
+	if (ret < 0) {
+		dev_err(mhdp->dev, "hdmi ctrl init failed = %d\n",  ret);
+		return;
+	}
+
+	/* Config GCP */

Quoted 19 lines · Sandor Yu

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +19 -0

+static int cdns_mhdp8501_read_hpd(struct cdns_mhdp8501_device *mhdp)
+{
+	u8 status;
+	int ret;
+
+	mutex_lock(&mhdp_mailbox_mutex);
+
+	ret = cdns_mhdp_mailbox_send(&mhdp->base, MB_MODULE_ID_GENERAL,
+				     GENERAL_GET_HPD_STATE, 0, NULL);
+	if (ret)
+		goto err_get_hpd;
+
+	ret = cdns_mhdp_mailbox_recv_header(&mhdp->base, MB_MODULE_ID_GENERAL,
+					    GENERAL_GET_HPD_STATE,
+					    sizeof(status));
+	if (ret)
+		goto err_get_hpd;
+
+	ret = cdns_mhdp_mailbox_recv_data(&mhdp->base, &status, sizeof(status));

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 41

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +4 -0

+	if (ret)
+		goto err_get_hpd;
+
+	mutex_unlock(&mhdp_mailbox_mutex);

That's better I guess, but it's still not a good API design. If you can't have concurrent accesses, then cdns_mhdp_mailbox_send et al. should take the mutex themselves.

Quoted 9 lines · Sandor Yu

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +8 -0

+static int cdns_mhdp8501_read_hpd(struct cdns_mhdp8501_device
+*mhdp) {
+   u8 status;
+   int ret;
+
+   mutex_lock(&mhdp_mailbox_mutex);
+
+   ret = cdns_mhdp_mailbox_send(&mhdp->base,

MB_MODULE_ID_GENERAL,

Quoted 1 line

Quoted 1 line

+ GENERAL_GET_HPD_STATE, 0,

NULL);

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +4 -0

+   if (ret)
+           goto err_get_hpd;
+
+   ret = cdns_mhdp_mailbox_recv_header(&mhdp->base,

MB_MODULE_ID_GENERAL,

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +6 -0

+                                       GENERAL_GET_HPD_STATE,
+                                       sizeof(status));
+   if (ret)
+           goto err_get_hpd;
+
+   ret = cdns_mhdp_mailbox_recv_data(&mhdp->base, &status,

sizeof(status));

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 41

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +4 -0

+   if (ret)
+           goto err_get_hpd;
+
+   mutex_unlock(&mhdp_mailbox_mutex);

That's better I guess, but it's still not a good API design. If you can't have concurrent accesses, then cdns_mhdp_mailbox_send et al. should take the mutex themselves.

I think that a proper API might be:

int cdns_mhdp_mailbox_send_recv(struct cdns_mhdp_device *mhdp,
u8 module_id, u8 opcode,
u16 size, const u8 *message, u16 buf_size, u8 *buf);

Internally it should take the lock, exchange the data, then return.

--
With best wishes
Dmitry

Quoted 1 line

+static int cdns_mhdp8501_read_hpd(struct cdns_mhdp8501_device

*mhdp)

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c

Quoted 7 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +7 -0

+{
+	u8 status;
+	int ret;
+
+	mutex_lock(&mhdp_mailbox_mutex);
+
+	ret = cdns_mhdp_mailbox_send(&mhdp->base,

MB_MODULE_ID_GENERAL,

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +5 -0

+				     GENERAL_GET_HPD_STATE, 0, NULL);
+	if (ret)
+		goto err_get_hpd;
+
+	ret = cdns_mhdp_mailbox_recv_header(&mhdp->base,

MB_MODULE_ID_GENERAL,

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +6 -0

+					    GENERAL_GET_HPD_STATE,
+					    sizeof(status));
+	if (ret)
+		goto err_get_hpd;
+
+	ret = cdns_mhdp_mailbox_recv_data(&mhdp->base, &status,

sizeof(status));

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 41

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +4 -0

+	if (ret)
+		goto err_get_hpd;
+
+	mutex_unlock(&mhdp_mailbox_mutex);

That's better I guess, but it's still not a good API design. If you can't have concurrent accesses, then cdns_mhdp_mailbox_send et al.
should take the mutex themselves.

Quoted 15 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +14 -0

+	return status;
+
+err_get_hpd:
+	dev_err(mhdp->dev, "read hpd  failed: %d\n", ret);
+	mutex_unlock(&mhdp_mailbox_mutex);
+
+	return ret;
+}
+
+enum drm_connector_status cdns_mhdp8501_detect(struct
+cdns_mhdp8501_device *mhdp) {
+	u8 hpd = 0xf;
+
+	hpd = cdns_mhdp8501_read_hpd(mhdp);

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 62

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +6 -0

+	if (hpd == 1)
+		return connector_status_connected;
+	else if (hpd == 0)
+		return connector_status_disconnected;
+
+	dev_warn(mhdp->dev, "Unknown cable status, hdp=%u\n", hpd);

This is already logged, there's no need to add a message there.

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +9 -0

+	return connector_status_unknown;
+}
+
+static void hotplug_work_func(struct work_struct *work) {
+	struct cdns_mhdp8501_device *mhdp = container_of(work,
+						     struct cdns_mhdp8501_device,
+						     hotplug_work.work);
+	enum drm_connector_status status = cdns_mhdp8501_detect(mhdp);
+

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 77

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +5 -0

+	drm_bridge_hpd_notify(&mhdp->bridge, status);
+
+	if (status == connector_status_connected) {
+		/* Cable connected  */
+		DRM_INFO("HDMI/DP Cable Plug In\n");

You might want to log it using drm_dev_debug, but anything else is a big stretch.

Quoted 22 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +21 -0

+		enable_irq(mhdp->irq[IRQ_OUT]);
+
+		/* Reset HDMI/DP link with sink */
+		if (mhdp->connector_type == DRM_MODE_CONNECTOR_HDMIA)
+			cdns_hdmi_reset_link(mhdp);
+		else
+			cdns_dp_check_link_state(mhdp);
+
+	} else if (status == connector_status_disconnected) {
+		/* Cable Disconnected  */
+		DRM_INFO("HDMI/DP Cable Plug Out\n");
+		enable_irq(mhdp->irq[IRQ_IN]);
+	}
+}
+
+static irqreturn_t cdns_mhdp8501_irq_thread(int irq, void *data) {
+	struct cdns_mhdp8501_device *mhdp = data;
+
+	disable_irq_nosync(irq);
+
+	mod_delayed_work(system_wq, &mhdp->hotplug_work,

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 103

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +4 -0

+			 msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));
+
+	return IRQ_HANDLED;
+}

disable_irq_nosync doesn't guarantee that you won't be called before interrupts are reenabled. What will happen if this handler is called multiple times?

Quoted 27 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +26 -0

+
+static int cdns_mhdp8501_dt_parse(struct cdns_mhdp8501_device *mhdp,
+				  struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *remote;
+
+	remote = of_graph_get_remote_node(np, 1, 0);
+	if (!remote) {
+		dev_err(dev, "fail to get remote node\n");
+		of_node_put(remote);
+		return -EINVAL;
+	}
+
+	/* get connector type */
+	if (of_device_is_compatible(remote, "hdmi-connector")) {
+		mhdp->connector_type = DRM_MODE_CONNECTOR_HDMIA;
+
+	} else if (of_device_is_compatible(remote, "dp-connector")) {
+		mhdp->connector_type = DRM_MODE_CONNECTOR_DisplayPort;
+
+	} else {
+		dev_err(dev, "Unknown connector type\n");
+		of_node_put(remote);
+		return -EINVAL;

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 135

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +6 -0

+	}
+
+	of_node_put(remote);
+
+	if (of_property_read_u32(np, "lane-mapping", &mhdp->lane_mapping)) {
+		dev_warn(dev, "Failed to get lane_mapping - using default\n");

debug logging as well.

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +4 -0

+		mhdp->lane_mapping = LANE_MAPPING_FLIPPED;
+	}
+
+	return true;

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +10 -0

+}
+
+static void cdns_mhdp8501_add_bridge(struct cdns_mhdp8501_device
+*mhdp) {
+	mhdp->bridge.type = mhdp->connector_type;
+	mhdp->bridge.driver_private = mhdp;
+	mhdp->bridge.of_node = mhdp->dev->of_node;
+	mhdp->bridge.vendor = "NXP";
+	mhdp->bridge.product = "i.MX8";
+	mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID

|

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +6 -0

+			   DRM_BRIDGE_OP_HPD;
+
+	if (mhdp->connector_type == DRM_MODE_CONNECTOR_HDMIA) {
+		mhdp->bridge.funcs = &cdns_hdmi_bridge_funcs;
+		mhdp->bridge.ops |= DRM_BRIDGE_OP_HDMI;
+	} else if (mhdp->connector_type ==

DRM_MODE_CONNECTOR_DisplayPort) {

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 160

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +5 -0

+		mhdp->bridge.funcs = &cdns_dp_bridge_funcs;
+	} else {
+		dev_err(mhdp->dev, "Unsupported connector type!\n");
+		return;
+	}

If this function can error out, then it should return an error code.

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +4 -0

+static int cdns_hdmi_scdc_write(struct cdns_mhdp8501_device *mhdp, u8
+addr, u8 value) {
+	u8 msg[5], reg[5];
+	int ret;

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +10 -0

+
+	msg[0] = 0x54;
+	msg[1] = addr;
+	msg[2] = 0;
+	msg[3] = 1;
+	msg[4] = value;
+
+	mutex_lock(&mhdp_mailbox_mutex);
+
+	ret = cdns_mhdp_mailbox_send(&mhdp->base,

MB_MODULE_ID_HDMI_TX, HDMI_TX_WRITE,

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +5 -0

+				     sizeof(msg), msg);
+	if (ret)
+		goto err_scdc_write;
+
+	ret = cdns_mhdp_mailbox_recv_header(&mhdp->base,

MB_MODULE_ID_HDMI_TX,

Quoted 14 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +14 -0

+					    HDMI_TX_WRITE, sizeof(reg));
+	if (ret)
+		goto err_scdc_write;
+
+	ret = cdns_mhdp_mailbox_recv_data(&mhdp->base, reg, sizeof(reg));
+	if (ret)
+		goto err_scdc_write;
+
+	if (reg[0] != 0)
+		ret = -EINVAL;
+
+err_scdc_write:
+
+	mutex_unlock(&mhdp_mailbox_mutex);

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 165

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +5 -0

+
+	if (ret)
+		dev_err(mhdp->dev, "scdc write failed: %d\n", ret);
+	return ret;
+}

You should be using SCDC helpers there instead of hand crafting your messages.

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 364

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +4 -0

+static void cdns_hdmi_sink_config(struct cdns_mhdp8501_device *mhdp)
+{
+	struct drm_display_info *display = &mhdp->curr_conn->display_info;
+	struct drm_connector_state *conn_state = mhdp->curr_conn->state;

That looks a bit hackish to me. We should probably provide a helper to get the connector state the bridge is attached to.

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +9 -0

+	struct drm_scdc *scdc = &mhdp->curr_conn->display_info.hdmi.scdc;
+	u8 buff = 0;
+
+	/* check sink type (HDMI or DVI) */
+	if (!display->is_hdmi) {
+		mhdp->hdmi.hdmi_type = MODE_DVI;
+		return;
+	}
+

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 379

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +6 -0

+	/* Default work in HDMI1.4 */
+	mhdp->hdmi.hdmi_type = MODE_HDMI_1_4;
+
+	/* check sink support SCDC or not */
+	if (!scdc->supported) {
+		dev_info(mhdp->dev, "Sink Not Support SCDC\n");

drm_dev_debug

Quoted 31 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +30 -0

+static enum drm_mode_status
+cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+			       const struct drm_display_mode *mode,
+			       unsigned long long tmds_rate) {
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	phy_cfg.hdmi.tmds_char_rate = tmds_rate;
+
+	ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0, &phy_cfg);
+	if (ret < 0)
+		return MODE_CLOCK_RANGE;
+
+	return MODE_OK;
+}
+
+static enum drm_mode_status
+cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+			    const struct drm_display_info *info,
+			    const struct drm_display_mode *mode) {
+	unsigned long long tmds_rate;
+
+	/* We don't support double-clocked and Interlaced modes */
+	if (mode->flags & DRM_MODE_FLAG_DBLCLK ||
+	    mode->flags & DRM_MODE_FLAG_INTERLACE)
+		return MODE_BAD;
+
+	if (mode->hdisplay > 3840)
+		return MODE_BAD_HVALUE;

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +6 -0

+
+	if (mode->vdisplay > 2160)
+		return MODE_BAD_VVALUE;
+
+	tmds_rate = mode->clock * 1000ULL;
+	return cdns_hdmi_tmds_char_rate_valid(bridge, mode, tmds_rate); }

Didn't we agree on creating a mode_valid helper?

Quoted 51 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +50 -0

+static void cdns_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
+					   struct drm_bridge_state *old_state) {
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	struct drm_atomic_state *state = old_state->base.state;
+	struct drm_connector *connector;
+	struct video_info *video_info = &mhdp->video_info;
+	struct drm_crtc_state *crtc_state;
+	struct drm_connector_state *conn_state;
+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	connector = drm_atomic_get_new_connector_for_encoder(state,
+							     bridge->encoder);
+	if (WARN_ON(!connector))
+		return;
+
+	mhdp->curr_conn = connector;
+
+	conn_state = drm_atomic_get_new_connector_state(state, connector);
+	if (WARN_ON(!conn_state))
+		return;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
+	if (WARN_ON(!crtc_state))
+		return;
+
+	video_info->color_fmt = conn_state->hdmi.output_format;
+	video_info->bpc = conn_state->hdmi.output_bpc;
+
+	drm_atomic_helper_connector_hdmi_update_infoframes(connector,
+state);
+
+	/* Line swapping */
+	cdns_mhdp_reg_write(&mhdp->base, LANES_CONFIG, 0x00400000 |
+mhdp->lane_mapping);
+
+	phy_cfg.hdmi.tmds_char_rate = conn_state->hdmi.tmds_char_rate;
+
+	ret = phy_configure(mhdp->phy, &phy_cfg);
+	if (ret) {
+		dev_err(mhdp->dev, "%s: phy_configure() failed: %d\n",
+			__func__, ret);
+		return;
+	}
+
+	cdns_hdmi_sink_config(mhdp);
+
+	ret = cdns_hdmi_ctrl_init(mhdp);
+	if (ret < 0) {
+		dev_err(mhdp->dev, "hdmi ctrl init failed = %d\n",  ret);

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +10 -0

+		return;
+	}
+
+	/* Config GCP */
+	if (video_info->bpc == 8)
+		cdns_hdmi_disable_gcp(mhdp);
+	else
+		cdns_hdmi_enable_gcp(mhdp);
+
+	ret = cdns_hdmi_mode_config(mhdp, &crtc_state->adjusted_mode,

video_info);

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (ret < 0) {
+		dev_err(mhdp->dev, "CDN_API_HDMITX_SetVic_blocking ret = %d\n",

ret);

Review

Quoted 2 lines

(quoted diff) +2 -0

+		return;
+	}

If the only thing you're doing is filling video_info and passing it there, why do you need that structure at all? Just pass the connector_state pointer.

Maxime

Quoted 7 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 364

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +4 -0

+static void cdns_hdmi_sink_config(struct cdns_mhdp8501_device *mhdp)
+{
+	struct drm_display_info *display = &mhdp->curr_conn->display_info;
+	struct drm_connector_state *conn_state = mhdp->curr_conn->state;

That looks a bit hackish to me. We should probably provide a helper to get the connector state the bridge is attached to.

How about code change to followed, is it more clear?

370         struct drm_connector *connector = mhdp->curr_conn;
371         struct drm_connector_state *conn_state = connector->state;
372         struct drm_display_info *display = &connector->display_info;
373         struct drm_scdc *scdc = &display->hdmi.scdc;

Quoted 30 lines

Quoted 30 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +30 -0

+static enum drm_mode_status
+cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+			       const struct drm_display_mode *mode,
+			       unsigned long long tmds_rate) {
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	phy_cfg.hdmi.tmds_char_rate = tmds_rate;
+
+	ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0, &phy_cfg);
+	if (ret < 0)
+		return MODE_CLOCK_RANGE;
+
+	return MODE_OK;
+}
+
+static enum drm_mode_status
+cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+			    const struct drm_display_info *info,
+			    const struct drm_display_mode *mode) {
+	unsigned long long tmds_rate;
+
+	/* We don't support double-clocked and Interlaced modes */
+	if (mode->flags & DRM_MODE_FLAG_DBLCLK ||
+	    mode->flags & DRM_MODE_FLAG_INTERLACE)
+		return MODE_BAD;
+
+	if (mode->hdisplay > 3840)
+		return MODE_BAD_HVALUE;

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +6 -0

+
+	if (mode->vdisplay > 2160)
+		return MODE_BAD_VVALUE;
+
+	tmds_rate = mode->clock * 1000ULL;
+	return cdns_hdmi_tmds_char_rate_valid(bridge, mode, tmds_rate); }

Didn't we agree on creating a mode_valid helper?

In fact, now I'm no idea where should add the mode_valid helper function.

In struct drm_bridge_funcs, it had mode_valid() and hdmi_tmds_char_rate_valid().

If create a new mode_valid helper function in struct drm_connector_hdmi_funcs, Is it appropriate to call another API function(tmds_char_rate_valid) at the same level within this API function?

Quoted 8 lines · Sandor Yu

Quoted 7 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 364

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +4 -0

+static void cdns_hdmi_sink_config(struct cdns_mhdp8501_device *mhdp)
+{
+	struct drm_display_info *display = &mhdp->curr_conn->display_info;
+	struct drm_connector_state *conn_state = mhdp->curr_conn->state;

That looks a bit hackish to me. We should probably provide a helper to get the connector state the bridge is attached to.

Quoted 5 lines

How about code change to followed, is it more clear?

370         struct drm_connector *connector = mhdp->curr_conn;
371         struct drm_connector_state *conn_state = connector->state;
372         struct drm_display_info *display = &connector->display_info;
373         struct drm_scdc *scdc = &display->hdmi.scdc;

What I meant was that I wish bridges had a way to get their connector pointer. It doesn't look like it's possible with drm_bridge_connector, and we don't have access to drm_display_info anymore.

I don't really see a good way to do this yet, so maybe that kind of workaround is ok. Eventually, I guess we'll have the scrambler setup in the HDMI connector helpers anyway.

Dmitry, any idea?

Quoted 40 lines

Quoted 31 lines

Quoted 31 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +30 -0

+static enum drm_mode_status
+cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+			       const struct drm_display_mode *mode,
+			       unsigned long long tmds_rate) {
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	phy_cfg.hdmi.tmds_char_rate = tmds_rate;
+
+	ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0, &phy_cfg);
+	if (ret < 0)
+		return MODE_CLOCK_RANGE;
+
+	return MODE_OK;
+}
+
+static enum drm_mode_status
+cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+			    const struct drm_display_info *info,
+			    const struct drm_display_mode *mode) {
+	unsigned long long tmds_rate;
+
+	/* We don't support double-clocked and Interlaced modes */
+	if (mode->flags & DRM_MODE_FLAG_DBLCLK ||
+	    mode->flags & DRM_MODE_FLAG_INTERLACE)
+		return MODE_BAD;
+
+	if (mode->hdisplay > 3840)
+		return MODE_BAD_HVALUE;

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +6 -0

+
+	if (mode->vdisplay > 2160)
+		return MODE_BAD_VVALUE;
+
+	tmds_rate = mode->clock * 1000ULL;
+	return cdns_hdmi_tmds_char_rate_valid(bridge, mode, tmds_rate); }

Didn't we agree on creating a mode_valid helper?

Quoted 7 lines

In fact, now I'm no idea where should add the mode_valid helper function.

In struct drm_bridge_funcs, it had mode_valid() and hdmi_tmds_char_rate_valid().

If create a new mode_valid helper function in struct drm_connector_hdmi_funcs, Is it appropriate to call another API function(tmds_char_rate_valid) at the same level within this API function?

I'm not quite sure what you mean, but a reasonable approach to me would be to turn drm_hdmi_state_helper.c hdmi_clock_valid into a public function, and then call it from drm_bridge_connector mode_valid hook.

It's a similar discussion to the previous one really: in order to implement it properly, we need access to drm_display_info.

Quoted 4 lines · Sandor Yu

Quoted 3 lines

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+static void cdns_hdmi_sink_config(struct cdns_mhdp8501_device
+*mhdp) {
+       struct drm_display_info *display =

&mhdp->curr_conn->display_info;

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+       struct drm_connector_state *conn_state =
+mhdp->curr_conn->state;

That looks a bit hackish to me. We should probably provide a helper to get the connector state the bridge is attached to.

How about code change to followed, is it more clear?
370 struct drm_connector *connector = mhdp->curr_conn;
371 struct drm_connector_state *conn_state =

connector->state;

Quoted 1 line

Quoted 1 line

372 struct drm_display_info *display =

&connector->display_info;

Quoted 11 lines

Quoted 1 line

373 struct drm_scdc *scdc = &display->hdmi.scdc;

What I meant was that I wish bridges had a way to get their connector pointer. It doesn't look like it's possible with drm_bridge_connector, and we don't have access to drm_display_info anymore.

I don't really see a good way to do this yet, so maybe that kind of workaround is ok. Eventually, I guess we'll have the scrambler setup in the HDMI connector helpers anyway.

Dmitry, any idea?

Unfortunately nothing significant, I didn't have time to look at the scrambler yet. The platforms that I'm working with either do not support HDMI 2.0 or require no additional setup there.

Regarding the drm_connector_state, I had to go via the bridge->encoder, then drm_atomic_get_new_connector_for_encoder() and finally drm_atomic_get_new_connector_state(). I don't like the idea of storing the connector in drm_bridge driver data, it seems like a bad idea to me.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +3 -0

+static enum drm_mode_status
+cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+                              const struct drm_display_mode

*mode,

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+                              unsigned long long tmds_rate) {
+       struct cdns_mhdp8501_device *mhdp =

bridge->driver_private;

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +6 -0

+       union phy_configure_opts phy_cfg;
+       int ret;
+
+       phy_cfg.hdmi.tmds_char_rate = tmds_rate;
+
+       ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0,

&phy_cfg);

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +10 -0

+       if (ret < 0)
+               return MODE_CLOCK_RANGE;
+
+       return MODE_OK;
+}
+
+static enum drm_mode_status
+cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+                           const struct drm_display_info *info,
+                           const struct drm_display_mode

*mode) {

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 3 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +3 -0

+       unsigned long long tmds_rate;
+
+       /* We don't support double-clocked and Interlaced modes

*/

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 13 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +13 -0

+       if (mode->flags & DRM_MODE_FLAG_DBLCLK ||
+           mode->flags & DRM_MODE_FLAG_INTERLACE)
+               return MODE_BAD;
+
+       if (mode->hdisplay > 3840)
+               return MODE_BAD_HVALUE;
+
+       if (mode->vdisplay > 2160)
+               return MODE_BAD_VVALUE;
+
+       tmds_rate = mode->clock * 1000ULL;
+       return cdns_hdmi_tmds_char_rate_valid(bridge, mode,
+ tmds_rate); }

Didn't we agree on creating a mode_valid helper?

Quoted 3 lines

Quoted 3 lines

In fact, now I'm no idea where should add the mode_valid helper function.

In struct drm_bridge_funcs, it had mode_valid() and

hdmi_tmds_char_rate_valid().

Quoted 8 lines

Quoted 4 lines

If create a new mode_valid helper function in struct drm_connector_hdmi_funcs, Is it appropriate to call another API function(tmds_char_rate_valid) at the same level within this API function?

I'm not quite sure what you mean, but a reasonable approach to me would be to turn drm_hdmi_state_helper.c hdmi_clock_valid into a public function, and then call it from drm_bridge_connector mode_valid

hook.

Quoted 3 lines

It's a similar discussion to the previous one really: in order to implement it properly, we need access to drm_display_info.

I've sent a proposal, [1]. I don't think we should be using hdmi_clock_valid() directly (at least not before sorting out the EDID / hotplug handling in the HDMI Connector code) exactly because of the info->max_tmds_clock. If it gets stale, we might filter modes incorrectly. Also, I'm not sure if it should be left at 0 by default (or in drm_parse_hdmi_forum_scds()).

[1]
https://lore.ke/
rnel.org%2Fdri-devel%2F20241018-hdmi-mode-valid-v1-0-6e49ae4801f7%40 linaro.org%2F&data=05%7C02%7Csandor.yu%40nxp.com%7Cb6594cde65474 3f8f1fe08dcf04effb5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7 C638649469314303142%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C &sdata=t3B356GQ5b7NYN1tspIuTmMLf3fQZCycbJ8UrSswnEk%3D&reserved =0

--
With best wishes
Dmitry

The patch set initial support Cadence MHDP8501(HDMI/DP) DRM bridge driver and Cadence HDP-TX PHY(HDMI/DP) driver for Freescale i.MX8MQ.

The patch set compose of DRM bridge drivers and PHY driver.

Both of them need by patch #1 and #2 to pass build.

DRM bridges driver patches:
#1: drm: bridge: Cadence: Creat mhdp helper driver #2: phy: Add HDMI configuration options
#3: dt-bindings: display: bridge: Add Cadence MHDP8501 #4: drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

PHY driver patches:
#1: drm: bridge: Cadence: Creat mhdp helper driver
#2: phy: Add HDMI configuration options
#5: dt-bindings: phy: Add Freescale iMX8MQ DP and HDMI PHY #6: phy: freescale: Add DisplayPort/HDMI Combo-PHY driver for i.MX8MQ

i.MX8M/TQMa8Mx DT patches:
#7: Add DT nodes for DCSS/HDMI pipeline
#8: Enable HDMI for TQMa8Mx/MBa8Mx

v16->v17:
Patch #1:
- Replaces the local mutex mbox_mutex with a global mutex mhdp_mailbox_mutex
Patch #2:
- remove hdmi.h
- add 2024 year to copyright
- Add r-b tag.
Patch #3:
- Add lane-mapping property.
Patch #4:
- Reset the HDMI/DP link when an HPD (Hot Plug Detect) event is detected
- Move the HDMI protocol settings from hdmi_ctrl_init() to a new function cdns_hdmi_set_hdmi_mode_type(), to align with the introduced link reset functionality.
- Implement logic to check the type of HDMI sink.
If the sink is not a hdmi display, set the default mode to DVI.
- Implement hdmi_reset_infoframe function
- Reorder certain bit definitions in the header file to follow a descending order.
- Add "lane-mapping" property for both HDMI and DP, remove platform data from driver.
lane-mapping should be setting in dts according different board layout.
- Remove variable mode in struct cdns_mhdp8501_device, video mode could get from struct drm_crtc_state
- Remove variable char_rate in struct cdns_mhdp8501_device, it could get from struct struct drm_connector_state.hdmi
- Replaces the local mutex mbox_mutex with a global mutex mhdp_mailbox_mutex
- Remove mutext protect for phy_api access functions.
Patch #6:
- Remove mbox_mutex

v15->v16:
Patch #2:
- Remove pixel_clk_rate, bpc and color_space fields from struct phy_configure_opts_hdmi, they were replaced by unsigned long long tmds_char_rate.
- Remove r-b and a-c tags because this patch have important change. Patch #4:
- Add DRM_BRIDGE_OP_HDMI flags for HDMI driver,
- Introduce the hdmi info frame helper functions,
added hdmi_clear_infoframe(), hdmi_write_infoframe() and hdmi_tmds_char_rate_valid() according Dmitry's patch 'make use of the HDMI connector infrastructure' patchset ([2]).
- mode_fixup() is replaced by atomic_check().
- Fix video mode 4Kp30 did not work on some displays that support LTE_340Mcsc_scramble.
- updated for tmds_char_rate added in patch #2.
Patch #6:
- updated for tmds_char_rate added in patch #2.

v14->v15:
Patch #6 + #7:
- Merged PHY driver into a single combo PHY driver Patch #7 + #8:
- Add DT patches for a running HDMI setup

v13->v14:
Patch #4:
- Rebase to next-20240219, replace get_edid function by edid_read function as commits d807ad80d811b ("drm/bridge: add ->edid_read hook and drm_bridge_edid_read()") and 27b8f91c08d99 ("drm/bridge:
remove ->get_edid callback") had change the API.

v12->v13:
Patch #4:
- Explicitly include linux/platform_device.h for cdns-mhdp8501-core.c
- Fix build warning
- Order bit bpc and color_space in descending shit.
Patch #7:
- Fix build warning

v11->v12:
Patch #1:
- Move status initialize out of mbox_mutex.
- Reorder API functions in alphabetical.
- Add notes for malibox access functions.
- Add year 2024 to copyright.
Patch #4:
- Replace DRM_INFO with dev_info or dev_warn.
- Replace DRM_ERROR with dev_err.
- Return ret when cdns_mhdp_dpcd_read failed in function cdns_dp_aux_transferi().
- Remove unused parmeter in function cdns_dp_get_msa_misc
and use two separate variables for color space and bpc.
- Add year 2024 to copyright.
Patch #6:
- Return error code to replace -1 for function wait_for_ack().
- Set cdns_phy->power_up = false in phy_power_down function.
- Remove "RATE_8_1 = 810000", it is not used in driver.
- Add year 2024 to copyright.
Patch #7:
- Adjust clk disable order.
- Return error code to replace -1 for function wait_for_ack().
- Use bool for variable pclk_in.
- Add year 2024 to copyright.

v10->v11:
- rewrite cdns_mhdp_set_firmware_active() in mhdp8546 core driver, use cdns_mhdp_mailbox_send() to replace cdns_mhdp_mailbox_write() same as the other mailbox access functions.
- use static for cdns_mhdp_mailbox_write() and cdns_mhdp_mailbox_read() and remove them from EXPORT_SYMBOL_GPL().
- remove MODULE_ALIAS() from mhdp8501 driver.

v9->v10:
- Create mhdp helper driver to replace macro functions, move all mhdp mailbox access functions and common functions into the helper driver.
Patch #1:drm: bridge: Cadence: Creat mhdp helper driver it is totaly different with v9.

v8->v9:
- Remove compatible string "cdns,mhdp8501" that had removed from dt-bindings file in v8.
- Add Dmitry's R-b tag to patch #2
- Add Krzysztof's R-b tag to patch #3

v7->v8:
MHDP8501 HDMI/DP:
- Correct DT node name to "display-bridge".
- Remove "cdns,mhdp8501" from mhdp8501 dt-binding doc.

HDMI/DP PHY:
- Introduced functions wait_for_ack and wait_for_ack_clear to handle waiting with acknowledgment bits set and cleared respectively.
- Use FIELD_PRE() to set bitfields for both HDMI and DP PHY.

v6->v7:
MHDP8501 HDMI/DP:
- Combine HDMI and DP driver into one mhdp8501 driver. Use the connector type to load the corresponding functions.
- Remove connector init functions.
- Add <linux/hdmi.h> in phy_hdmi.h to reuse 'enum hdmi_colorspace'.

HDMI/DP PHY:
- Lowercase hex values
- Fix parameters indent issue on some functions
- Replace 'udelay' with 'usleep_range'

v5->v6:
HDMI/DP bridge driver
- 8501 is the part number of Cadence MHDP on i.MX8MQ.
Use MHDP8501 to name hdmi/dp drivers and files.
- Add compatible "fsl,imx8mq-mhdp8501-dp" for i.MX8MQ DP driver
- Add compatible "fsl,imx8mq-mhdp8501-hdmi" for i.MX8MQ HDMI driver
- Combine HDMI and DP dt-bindings into one file cdns,mhdp8501.yaml
- Fix HDMI scrambling is not enable issue when driver working in 4Kp60 mode.
- Add HDMI/DP PHY API mailbox protect.

HDMI/DP PHY driver:
- Rename DP and HDMI PHY files and move to folder phy/freescale/
- Remove properties num_lanes and link_rate from DP PHY driver.
- Combine HDMI and DP dt-bindings into one file fsl,imx8mq-dp-hdmi-phy.yaml
- Update compatible string to "fsl,imx8mq-dp-phy".
- Update compatible string to "fsl,imx8mq-hdmi-phy".

v4->v5:
- Drop "clk" suffix in clock name.
- Add output port property in the example of hdmi/dp.

v3->v4:
dt-bindings:
- Correct dt-bindings coding style and address review comments.
- Add apb_clk description.
- Add output port for HDMI/DP connector
PHY:
- Alphabetically sorted in Kconfig and Makefile for DP and HDMI PHY
- Remove unused registers define from HDMI and DP PHY drivers.
- More description in phy_hdmi.h.
- Add apb_clk to HDMI and DP phy driver.
HDMI/DP:
- Use get_unaligned_le32() to replace hardcode type conversion in HDMI AVI infoframe data fill function.
- Add mailbox mutex lock in HDMI/DP driver for phy functions to reslove race conditions between HDMI/DP and PHY drivers.
- Add apb_clk to both HDMI and DP driver.
- Rename some function names and add prefix with "cdns_hdmi/cdns_dp".
- Remove bpc 12 and 16 optional that not supported.

v2->v3:
Address comments for dt-bindings files.
- Correct dts-bindings file names
Rename phy-cadence-hdptx-dp.yaml to cdns,mhdp-imx8mq-dp.yaml Rename phy-cadence-hdptx-hdmi.yaml to cdns,mhdp-imx8mq-hdmi.yaml
- Drop redundant words and descriptions.
- Correct hdmi/dp node name.

v2 is a completely different version compared to v1. Previous v1 can be available here [1].

v1->v2:
- Reuse Cadence mailbox access functions from mhdp8546 instead of rockchip DP.
- Mailbox access functions be convert to marco functions that will be referenced by HDP-TX PHY(HDMI/DP) driver too.
- Plain bridge instead of component driver.
- Standalone Cadence HDP-TX PHY(HDMI/DP) driver.
- Audio driver are removed from the patch set, it will be add in another patch set later.

[1] https://patchwork.kernel.org/project/linux-rockchip/cover/[email protected]/

Alexander Stein (2):
arm64: dts: imx8mq: Add DCSS + HDMI/DP display pipeline
arm64: dts: imx8mq: tqma8mq-mba8mx: Enable HDMI support

Sandor Yu (6):
drm: bridge: Cadence: Create mhdp helper driver
phy: Add HDMI configuration options
dt-bindings: display: bridge: Add Cadence MHDP8501
drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver
dt-bindings: phy: Add Freescale iMX8MQ DP and HDMI PHY
phy: freescale: Add DisplayPort/HDMI Combo-PHY driver for i.MX8MQ

 .../display/bridge/cdns,mhdp8501.yaml         |  109 ++
 .../bindings/phy/fsl,imx8mq-dp-hdmi-phy.yaml  |   51 +
 .../dts/freescale/imx8mq-tqma8mq-mba8mx.dts   |   20 +
 arch/arm64/boot/dts/freescale/imx8mq.dtsi     |   68 +
 arch/arm64/boot/dts/freescale/mba8mx.dtsi     |   11 +
 drivers/gpu/drm/bridge/cadence/Kconfig        |   20 +
 drivers/gpu/drm/bridge/cadence/Makefile       |    3 +
 .../gpu/drm/bridge/cadence/cdns-mhdp-helper.c |  307 ++++
 .../drm/bridge/cadence/cdns-mhdp8501-core.c   |  323 ++++
 .../drm/bridge/cadence/cdns-mhdp8501-core.h   |  359 +++++
 .../gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c |  737 +++++++++
 .../drm/bridge/cadence/cdns-mhdp8501-hdmi.c   |  692 +++++++++
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   |  425 +-----
 .../drm/bridge/cadence/cdns-mhdp8546-core.h   |   47 +-
 .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   |   36 +-
 drivers/phy/freescale/Kconfig                 |   10 +
 drivers/phy/freescale/Makefile                |    1 +
 drivers/phy/freescale/phy-fsl-imx8mq-hdptx.c  | 1337 +++++++++++++++++
 include/drm/bridge/cdns-mhdp-helper.h         |   94 ++
 include/linux/phy/phy-hdmi.h                  |   19 +
 include/linux/phy/phy.h                       |    7 +-
 21 files changed, 4268 insertions(+), 408 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml
 create mode 100644 Documentation/devicetree/bindings/phy/fsl,imx8mq-dp-hdmi-phy.yaml
 create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
 create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c
 create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.h
 create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c
 create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c
 create mode 100644 drivers/phy/freescale/phy-fsl-imx8mq-hdptx.c
 create mode 100644 include/drm/bridge/cdns-mhdp-helper.h
 create mode 100644 include/linux/phy/phy-hdmi.h

--
2.34.1