linux-kernel - Re: [PATCH v2 01/12] drm: bridge/dw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150131110704.GU26493@n2100.arm.linux.org.uk>
Date:	Sat, 31 Jan 2015 11:07:04 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Yakir Yang <ykk@...k-chips.com>
Cc:	David Airlie <airlied@...ux.ie>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Fabio Estevam <fabio.estevam@...escale.com>,
	Shawn Guo <shawn.guo@...aro.org>,
	Rob Clark <robdclark@...il.com>,
	Mark Yao <mark.yao@...k-chips.com>,
	Daniel Vetter <daniel@...ll.ch>,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
	djkurtz@...omium.org, dbehr@...omoum.org, mmind00@...glemail.com,
	dianders@...omium.org, marcheu@...omium.org,
	rockchip-discuss@...omium.org
Subject: Re: [PATCH v2 01/12] drm: bridge/dw_hdmi: adjust n/cts setting order

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 <ykk@...k-chips.com>
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/