Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 12:55:58PM +0000, Jose Abreu wrote:
> Hi,
> 
> 
> On 13-03-2017 09:36, Russell King - ARM Linux wrote:
> > On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
> >> On 03/10/2017 10:35 AM, Romain Perier wrote:
> >>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
> >>> step E. and is kept enabled for later use. This clock should be enabled
> >>> and disabled along with the actual audio stream and not always on (that
> >>> is bad for PM). Futhermore, this might cause sound glitches with some
> >>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
> >>> while the audio clock is still running.
> >>>
> >>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
> >>> when the audio sample clock must be enabled or disabled. Then, it moves
> >>> the call to this function into dw_hdmi_audio_enable() and
> >>> dw_hdmi_audio_disable().
> >>>
> >>> Signed-off-by: Romain Perier <romain.perier@xxxxxxxxxxxxx>
> >>> ---
> >>>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
> >>>  1 file changed, 9 insertions(+), 6 deletions(-)
> >>>
> >> Hi Romain, Russell, Jose,
> >>
> >> This is a little out of scope, but I was wondering why the CTS calculation
> >> was not left in AUTO mode in the dw-hdmi driver ?
> > There is no indication in the iMX6 manuals that the iMX6 supports
> > automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
> > marked as "reserved".
> >
> > We're reliant on the information in the iMX6 manuals as we don't have
> > access to Synopsis' databooks for these parts (I understand you have
> > to be a customer to have access to that.)
> >
> 
> (Synopsis -> Synopsys :))
> 
> Trying to catch up with the conversation:
> 
> 1) In AHB audio mode the bits are always reserved.
> 2) I think we should enable/disable clock instead of just forcing
> N/CTS, though, I don't know what could be the implications for
> iMX platform. I remember at the time I tested this using I2S
> (I've never used AHB), and HDMI protocol analyzers were
> complaining about the N/CTS being forced to zero.

What you're recommending is to basically ignore the published workaround,
which, presumably, was arrived at by proper analysis of the issue both
by Freescale engineers and Synopsys engineers, and instead try some other
solution.

I'm not sure that's a good idea.  Only those with knowledge of the IP can
say what effect disabling the audio clock will have: will it still allow
the FIFO to be loaded, as required by the errata?  If not, it's not a
solution that AHB can use.  I would imagine that _if_ it was as simple as
disabling the audio clock, that simple approach would have been documented
in Freescale's errata documents, rather than the rather more complex
method of zeroing the CTS/N values.

I think there are two choices here:

1) get some definitive information about the IP and the errata for AHB,
   and determine whether the solution you propose would work.  (That's
   out of scope for me, I've no contacts with FSL/NXP and I'm not a
   Synopsys customer.)

2) the I2S audio support stops re-using the AHB audio support functions,
   switching to their own implementation that behaves correctly for them.
   (Remember, it was I2S's choice to re-use the AHB audio support functions
   I added which we're now discussing - they didn't have to do that, and
   regressing AHB audio just for I2S is not something we should ever
   consider doing.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.