Re: [PATCH v2 2/2] drm: bridge: cdns-mhdp8546: Add support for no-hpd
From: Jayesh Choudhary
Date: Fri Apr 14 2023 - 11:11:03 EST
On 06/04/23 07:22, Laurent Pinchart wrote:
Hi Jayesh,
Thank you for the patch.
On Wed, Apr 05, 2023 at 07:54:40PM +0530, Jayesh Choudhary wrote:
From: Rahul T R <r-ravikumar@xxxxxx>
In J721S2 EVMs DP0 hpd is not connected to correct
hpd pin on SOC, to handle such cases, Add support for
"no-hpd" property in the device tree node to disable
hpd
s/hpd/hpd./
You can also reflow the commit message to 72 columns.
Okay. Thanks for the suggestion. Will do.
Also change the log level for dpcd read failuers to
debug, since framework retries 32 times for each read
s/read/read./
Doesn't this apply to writes as well ?
Based on message request, we went into the conditional that uses
read. So just changing the log-level for dpcd read was enough to
get rid of the debug logs.
Signed-off-by: Rahul T R <r-ravikumar@xxxxxx>
Signed-off-by: Jayesh Choudhary <j-choudhary@xxxxxx>
---
.../drm/bridge/cadence/cdns-mhdp8546-core.c | 37 ++++++++++++++++---
.../drm/bridge/cadence/cdns-mhdp8546-core.h | 1 +
2 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index f6822dfa3805..e177794b069d 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -54,6 +54,8 @@
#include "cdns-mhdp8546-hdcp.h"
#include "cdns-mhdp8546-j721e.h"
+static int cdns_mhdp_update_link_status(struct cdns_mhdp_device *mhdp);
+
static int cdns_mhdp_mailbox_read(struct cdns_mhdp_device *mhdp)
{
int ret, empty;
@@ -749,7 +751,7 @@ static int cdns_mhdp_fw_activate(const struct firmware *fw,
* MHDP_HW_STOPPED happens only due to driver removal when
* bridge should already be detached.
*/
- if (mhdp->bridge_attached)
+ if (mhdp->bridge_attached && !mhdp->no_hpd)
writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
mhdp->regs + CDNS_APB_INT_MASK);
@@ -845,7 +847,7 @@ static ssize_t cdns_mhdp_transfer(struct drm_dp_aux *aux,
ret = cdns_mhdp_dpcd_read(mhdp, msg->address,
msg->buffer, msg->size);
if (ret) {
- dev_err(mhdp->dev,
+ dev_dbg(mhdp->dev,
"Failed to read DPCD addr %u\n",
msg->address);
@@ -1738,6 +1740,19 @@ static int cdns_mhdp_attach(struct drm_bridge *bridge,
spin_unlock(&mhdp->start_lock);
+ if (mhdp->no_hpd) {
+ ret = wait_event_timeout(mhdp->fw_load_wq,
+ mhdp->hw_state == MHDP_HW_READY,
+ msecs_to_jiffies(100));
+ if (ret == 0) {
+ dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n",
+ __func__);
+ return -ETIMEDOUT;
+ }
+
+ cdns_mhdp_update_link_status(mhdp);
+ return 0;
+ }
Missing blank line.
It's not clear to me while you need to wait for the state to change to
MHDP_HW_READY in the no_hpd case. This should be explained in the commit
message.
/* Enable SW event interrupts */
if (hw_ready)
writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
@@ -2256,7 +2271,16 @@ static int cdns_mhdp_update_link_status(struct cdns_mhdp_device *mhdp)
mutex_lock(&mhdp->link_mutex);
- mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
+ if (mhdp->no_hpd) {
+ ret = drm_dp_dpcd_read_link_status(&mhdp->aux, status);
+ hpd_pulse = false;
+ if (ret < 0)
+ mhdp->plugged = false;
+ else
+ mhdp->plugged = true;
I think there's an issue with how the driver uses mhdp->plugged. In the
no_hpd case, you try to detect if a display is connected by reading the
link status at attach time, and then never update mhdp->plugged. This
means that if no display is connected at that point, functions like
cdns_mhdp_get_edid() will always fail, even if a display gets plugged
later. As the goal of this series is (as far as I understand) support
systems where the HPD signal could be connected to a SoC GPIO instead of
the bridge, I don't think this is good enough.
In the driver, I see that this is the only call which changes
mhdp->plugged. Do you have any suggestions on how to work on this?
Polling the value of drm_dp_dpdc_read_link_status does not seem like a
clean way.
Here by doing this, we are settling for few functionalities of display.
Thanks,
-Jayesh
+ } else {
+ mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
+ }
if (!mhdp->plugged) {
cdns_mhdp_link_down(mhdp);
@@ -2451,6 +2475,8 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
mhdp->aux.dev = dev;
mhdp->aux.transfer = cdns_mhdp_transfer;
+ mhdp->no_hpd = of_property_read_bool(dev->of_node, "cdns,no-hpd");
+
mhdp->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(mhdp->regs)) {
dev_err(dev, "Failed to get memory resource\n");
@@ -2526,8 +2552,9 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
mhdp->bridge.of_node = pdev->dev.of_node;
mhdp->bridge.funcs = &cdns_mhdp_bridge_funcs;
- mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
- DRM_BRIDGE_OP_HPD;
+ mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
+ if (!mhdp->no_hpd)
+ mhdp->bridge.ops |= DRM_BRIDGE_OP_HPD;
mhdp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
if (mhdp->info)
mhdp->bridge.timings = mhdp->info->timings;
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
index bedddd510d17..6786ccb51387 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
@@ -388,6 +388,7 @@ struct cdns_mhdp_device {
bool link_up;
bool plugged;
+ bool no_hpd;
/*
* "start_lock" protects the access to bridge_attached and