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

[Neil Armstrong]

On 03/14/2017 08:53 AM, Romain Perier wrote:
> Hi,
> 
> 
> Le 13/03/2017 à 19:49, Jose Abreu a écrit :
>> Hi Russell,
>>
>>
>> On 13-03-2017 13:10, Russell King - ARM Linux wrote:
>>> 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'm just following what the user guide says: the final step of
>> configuration is enabling the audio clock. There is no reference
>> neither in datasheet neither in user guide about this behavior
>> but, as I said, I've never used AHB so I can't prove what is the
>> best solution. Could it be platform specific?
> 
> And that's precisely why I am enabling it
> 
>>
>>> 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.)
>> This is the right thing to do but if you can't test in the
>> Freescale platform then we have not much else to do.
>>
>> Best regards,
>> Jose Miguel Abreu
>>
>>> 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.)
>>>
> 
> The workaround looks AHB specific in any cases and does not seem to work
> with I2S. The I2S variant should not used the same functions, at least
> for enabling/disabling audio stream.
> 
> Regards,
> Romain
> 
Hi All,

Jose, thanks for your clarification.

It seems reasonable to indeed add I2S/SPDIF specific functions, or at least add
parameters to change the behavior when the audio clock is generated by the
Synopsys IP or by an external audio block as in I2S and SPDIF.

Nevertheless, on the Amlogic platforms, we will need to add this AUTO cts calculation
feature and will certainly need this behavior fix.

Romain, feel free to add the correct I2S behavior, I'll be happy to test your patches.

Thanks,
Neil