| Message ID | 20240216124043.1226713-1-rohit.visavalia@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: xlnx: dp: Reset DisplayPort IP | expand |
[AMD Official Use Only - General] Hi Rohit, Thanks for the patch. > -----Original Message----- > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of > Rohit Visavalia > Sent: Friday, February 16, 2024 1:41 PM > To: gregkh@linuxfoundation.org; laurent.pinchart@ideasonboard.com; > maarten.lankhorst@linux.intel.com; mripard@kernel.org; > tzimmermann@suse.de; airlied@gmail.com; daniel@ffwll.ch; Simek, Michal > <michal.simek@amd.com>; dri-devel@lists.freedesktop.org; linux-arm- > kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Subject: [PATCH] drm: xlnx: dp: Reset DisplayPort IP > > Assert DisplayPort reset signal before deasserting, > it is to clear out any registers programmed before booting kernel. > > Signed-off-by: Rohit Visavalia <rohit.visavalia@amd.com> > --- > drivers/gpu/drm/xlnx/zynqmp_dp.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c > b/drivers/gpu/drm/xlnx/zynqmp_dp.c > index 1846c4971fd8..5a40aa1d4283 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > @@ -1714,6 +1714,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub > *dpsub) > goto err_free; > } > > + ret = zynqmp_dp_reset(dp, true); > + if (ret < 0) > + return ret; > + > ret = zynqmp_dp_reset(dp, false); > if (ret < 0) > goto err_free; > -- > 2.25.1 This looks good to me. Reviewed-by: Vishal Sagar<vishal.sagar@amd.com> Regards Vishal Sagar
Hello Rohit, Thank you for the patch. On Fri, Feb 16, 2024 at 04:40:43AM -0800, Rohit Visavalia wrote: > Assert DisplayPort reset signal before deasserting, > it is to clear out any registers programmed before booting kernel. > > Signed-off-by: Rohit Visavalia <rohit.visavalia@amd.com> > --- > drivers/gpu/drm/xlnx/zynqmp_dp.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c > index 1846c4971fd8..5a40aa1d4283 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > @@ -1714,6 +1714,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub) > goto err_free; > } > > + ret = zynqmp_dp_reset(dp, true); > + if (ret < 0) > + return ret; > + This looks fine to me, so Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> However, looking at zynqmp_dp_reset(), we have static int zynqmp_dp_reset(struct zynqmp_dp *dp, bool assert) { unsigned long timeout; if (assert) reset_control_assert(dp->reset); else reset_control_deassert(dp->reset); /* Wait for the (de)assert to complete. */ timeout = jiffies + msecs_to_jiffies(RST_TIMEOUT_MS); while (!time_after_eq(jiffies, timeout)) { bool status = !!reset_control_status(dp->reset); if (assert == status) return 0; cpu_relax(); } dev_err(dp->dev, "reset %s timeout\n", assert ? "assert" : "deassert"); return -ETIMEDOUT; } That doesn't seem quite right. Aren't reset_control_assert() and reset_control_deassert() supposed to be synchronous ? If so there should be no need to wait, and if there's a need to wait, it could be a bug in the reset controller driver. This should be fixed, and then the code in probe could be replaced with a call to reset_control_reset(). > ret = zynqmp_dp_reset(dp, false); > if (ret < 0) > goto err_free;
Tomi, could you push this through drm-misc ? On Wed, Feb 28, 2024 at 06:22:25PM +0200, Laurent Pinchart wrote: > Hello Rohit, > > Thank you for the patch. > > On Fri, Feb 16, 2024 at 04:40:43AM -0800, Rohit Visavalia wrote: > > Assert DisplayPort reset signal before deasserting, > > it is to clear out any registers programmed before booting kernel. > > > > Signed-off-by: Rohit Visavalia <rohit.visavalia@amd.com> > > --- > > drivers/gpu/drm/xlnx/zynqmp_dp.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > index 1846c4971fd8..5a40aa1d4283 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > @@ -1714,6 +1714,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub) > > goto err_free; > > } > > > > + ret = zynqmp_dp_reset(dp, true); > > + if (ret < 0) > > + return ret; > > + > > This looks fine to me, so > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > However, looking at zynqmp_dp_reset(), we have > > static int zynqmp_dp_reset(struct zynqmp_dp *dp, bool assert) > { > unsigned long timeout; > > if (assert) > reset_control_assert(dp->reset); > else > reset_control_deassert(dp->reset); > > /* Wait for the (de)assert to complete. */ > timeout = jiffies + msecs_to_jiffies(RST_TIMEOUT_MS); > while (!time_after_eq(jiffies, timeout)) { > bool status = !!reset_control_status(dp->reset); > > if (assert == status) > return 0; > > cpu_relax(); > } > > dev_err(dp->dev, "reset %s timeout\n", assert ? "assert" : "deassert"); > return -ETIMEDOUT; > } > > That doesn't seem quite right. Aren't reset_control_assert() and > reset_control_deassert() supposed to be synchronous ? If so there should > be no need to wait, and if there's a need to wait, it could be a bug in > the reset controller driver. This should be fixed, and then the code in > probe could be replaced with a call to reset_control_reset(). > > > ret = zynqmp_dp_reset(dp, false); > > if (ret < 0) > > goto err_free;
On 29/02/2024 11:05, Laurent Pinchart wrote: > Tomi, could you push this through drm-misc ? > > On Wed, Feb 28, 2024 at 06:22:25PM +0200, Laurent Pinchart wrote: >> Hello Rohit, >> >> Thank you for the patch. >> >> On Fri, Feb 16, 2024 at 04:40:43AM -0800, Rohit Visavalia wrote: >>> Assert DisplayPort reset signal before deasserting, >>> it is to clear out any registers programmed before booting kernel. >>> >>> Signed-off-by: Rohit Visavalia <rohit.visavalia@amd.com> >>> --- >>> drivers/gpu/drm/xlnx/zynqmp_dp.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c >>> index 1846c4971fd8..5a40aa1d4283 100644 >>> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c >>> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c >>> @@ -1714,6 +1714,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub) >>> goto err_free; >>> } >>> >>> + ret = zynqmp_dp_reset(dp, true); >>> + if (ret < 0) >>> + return ret; >>> + >> >> This looks fine to me, so >> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Thanks, tested and pushed to drm-misc-next. Tomi
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c index 1846c4971fd8..5a40aa1d4283 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c @@ -1714,6 +1714,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub) goto err_free; } + ret = zynqmp_dp_reset(dp, true); + if (ret < 0) + return ret; + ret = zynqmp_dp_reset(dp, false); if (ret < 0) goto err_free;
Assert DisplayPort reset signal before deasserting, it is to clear out any registers programmed before booting kernel. Signed-off-by: Rohit Visavalia <rohit.visavalia@amd.com> --- drivers/gpu/drm/xlnx/zynqmp_dp.c | 4 ++++ 1 file changed, 4 insertions(+)