Re: [PATCH v2 01/12] drm: bridge/dw_hdmi: adjust n/cts setting order

[Yang Kuankuan]

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 <ykk@xxxxxxxxxxxxxx>
> > ---
> > 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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/