From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753970AbbAaLH1 (ORCPT ); Sat, 31 Jan 2015 06:07:27 -0500 Received: from pandora.arm.linux.org.uk ([78.32.30.218]:38541 "EHLO pandora.arm.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750967AbbAaLHW (ORCPT ); Sat, 31 Jan 2015 06:07:22 -0500 Date: Sat, 31 Jan 2015 11:07:04 +0000 From: Russell King - ARM Linux To: Yakir Yang Cc: David Airlie , Philipp Zabel , Fabio Estevam , Shawn Guo , Rob Clark , Mark Yao , Daniel Vetter , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, djkurtz@chromium.org, dbehr@chromoum.org, mmind00@googlemail.com, dianders@chromium.org, marcheu@chromium.org, rockchip-discuss@chromium.org Subject: Re: [PATCH v2 01/12] drm: bridge/dw_hdmi: adjust n/cts setting order Message-ID: <20150131110704.GU26493@n2100.arm.linux.org.uk> References: <1422617031-25098-1-git-send-email-ykk@rock-chips.com> <1422617130-25187-1-git-send-email-ykk@rock-chips.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1422617130-25187-1-git-send-email-ykk@rock-chips.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 30, 2015 at 06:25:30AM -0500, Yakir Yang wrote: > For Designerware HDMI, the following write sequence is recommended: > 1. aud_n3 (set bit ncts_atomic_write if desired) > 2. aud_cts3 (set CTS_manual and CTS value if desired/enabled) > 3. aud_cts2 (required in CTS_manual) > 4. aud_cts1 (required in CTS_manual) > 5. aud_n3 (bit ncts_atomic_write with same value as in step 1.) > 6. aud_n2 > 7. aud_n1 > > Signed-off-by: Yakir Yang > --- > Changes in v2: > - adjust n/cts setting order > > drivers/gpu/drm/bridge/dw_hdmi.c | 37 +++++++++++++++++++++---------------- > drivers/gpu/drm/bridge/dw_hdmi.h | 6 ++++++ > 2 files changed, 27 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c > index cd6a706..423addc 100644 > --- a/drivers/gpu/drm/bridge/dw_hdmi.c > +++ b/drivers/gpu/drm/bridge/dw_hdmi.c > @@ -177,26 +177,31 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg, > hdmi_modb(hdmi, data << shift, mask, reg); > } > > -static void hdmi_set_clock_regenerator_n(struct dw_hdmi *hdmi, > - unsigned int value) > +static void hdmi_regenerate_n_cts(struct dw_hdmi *hdmi, unsigned int n, > + unsigned int cts) > { > - hdmi_writeb(hdmi, value & 0xff, HDMI_AUD_N1); > - hdmi_writeb(hdmi, (value >> 8) & 0xff, HDMI_AUD_N2); > - hdmi_writeb(hdmi, (value >> 16) & 0x0f, HDMI_AUD_N3); > + /* set ncts_atomic_write */ > + hdmi_modb(hdmi, HDMI_AUD_N3_NCTS_ATOMIC_WRITE_SET, > + HDMI_AUD_N3_NCTS_ATOMIC_WRITE_MASK, HDMI_AUD_N3); Bits 7:4 of N3 are marked up in the iMX6 manuals as "reserved". We need some clarification from FSL whether this matters, but at the moment my opinion on that is this should be conditionalised against the IP version. Setting bit 7 as you do above may not cause any harm on iMX6, but on the other hand, we shouldn't be setting bits which are marked as reserved. > + > + /* set cts manual */ > + hdmi_modb(hdmi, HDMI_AUD_CTS3_CTS_MANUAL, > + HDMI_AUD_CTS3_CTS_MANUAL, HDMI_AUD_CTS3); > > /* nshift factor = 0 */ > hdmi_modb(hdmi, 0, HDMI_AUD_CTS3_N_SHIFT_MASK, HDMI_AUD_CTS3); > -} > - > -static void hdmi_regenerate_cts(struct dw_hdmi *hdmi, unsigned int cts) > -{ > - /* Must be set/cleared first */ > - hdmi_modb(hdmi, 0, HDMI_AUD_CTS3_CTS_MANUAL, HDMI_AUD_CTS3); > > - hdmi_writeb(hdmi, cts & 0xff, HDMI_AUD_CTS1); > + /* set cts values */ > + hdmi_modb(hdmi, ((cts >> 16) & HDMI_AUD_CTS3_AUDCTS19_16_MASK), > + HDMI_AUD_CTS3_AUDCTS19_16_MASK, HDMI_AUD_CTS3); > hdmi_writeb(hdmi, (cts >> 8) & 0xff, HDMI_AUD_CTS2); > - hdmi_writeb(hdmi, ((cts >> 16) & HDMI_AUD_CTS3_AUDCTS19_16_MASK) | > - HDMI_AUD_CTS3_CTS_MANUAL, HDMI_AUD_CTS3); > + hdmi_writeb(hdmi, cts & 0xff, HDMI_AUD_CTS1); > + > + /* set n values */ > + hdmi_modb(hdmi, (n >> 16) & HDMI_AUD_N3_AUDN19_16_MASK, > + HDMI_AUD_N3_AUDN19_16_MASK, HDMI_AUD_N3); > + hdmi_writeb(hdmi, (n >> 8) & 0xff, HDMI_AUD_N2); > + hdmi_writeb(hdmi, n & 0xff, HDMI_AUD_N1); This is definitely moving things in the right direction. However, I would ask that the read-modify-writes to HDMI_AUD_N3 are open-coded rather than using hdmi_modb(), and consolidated. We really don't need to keep re-reading this register. I'd also like to check that this does not cause a regression on iMX6 SoCs with my audio driver; I'm aware that there are some issues to do with how these registers are written, and the published errata workaround in the iMX6 errata documentation doesn't seem to always work. > } > > static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk, > @@ -355,8 +360,8 @@ static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi, > __func__, hdmi->sample_rate, hdmi->ratio, > pixel_clk, clk_n, clk_cts); > > - hdmi_set_clock_regenerator_n(hdmi, clk_n); > - hdmi_regenerate_cts(hdmi, clk_cts); > + hdmi_regenerate_n_cts(hdmi, clk_n, clk_cts); > + hdmi_set_schnl(hdmi); I'd prefer the addition of hdmi_set_schnl() to be in the patch which adds that new function. However, as I mentioned in my reply to that patch, other versions of this IP do not have these registers, and it may not be safe to write to them. We need to find a way to know whether this IP has the AHB DMA support or not and act accordingly. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.