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:
- aud_n3 (set bit ncts_atomic_write if desired)
- aud_cts3 (set CTS_manual and CTS value if desired/enabled)
- aud_cts2 (required in CTS_manual)
- aud_cts1 (required in CTS_manual)
- aud_n3 (bit ncts_atomic_write with same value as in step 1.)
- aud_n2
- aud_n1
Signed-off-by: Yakir Yang ykk@rock-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.
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 >> 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.