From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933410AbbBBNDS (ORCPT ); Mon, 2 Feb 2015 08:03:18 -0500 Received: from regular1.263xmail.com ([211.150.99.138]:40943 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932102AbbBBNDO (ORCPT ); Mon, 2 Feb 2015 08:03:14 -0500 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: ykk@rock-chips.com X-FST-TO: rockchip-discuss@chromium.org X-SENDER-IP: 192.253.240.203 X-LOGIN-NAME: ykk@rock-chips.com X-UNIQUE-TAG: X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <54CF756D.1010500@rock-chips.com> Date: Mon, 02 Feb 2015 08:02:37 -0500 From: Yang Kuankuan User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Russell King - ARM Linux 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 References: <1422617031-25098-1-git-send-email-ykk@rock-chips.com> <1422617130-25187-1-git-send-email-ykk@rock-chips.com> <20150131110704.GU26493@n2100.arm.linux.org.uk> In-Reply-To: <20150131110704.GU26493@n2100.arm.linux.org.uk> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/31/2015 06:07 AM, Russell King - ARM Linux wrote: > 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. Should we take the design_id(0x13 on iMX6, 0x20 on rk3288) to separate it. > 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. Maybe we also can take design_id to separate it here. > > 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. >