From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750877AbdCNINh (ORCPT ); Tue, 14 Mar 2017 04:13:37 -0400 Received: from mail-wm0-f42.google.com ([74.125.82.42]:38245 "EHLO mail-wm0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750824AbdCNINg (ORCPT ); Tue, 14 Mar 2017 04:13:36 -0400 Subject: Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions To: Romain Perier , Jose Abreu , Russell King - ARM Linux References: <20170310093509.19044-1-romain.perier@collabora.com> <25ad96a7-d907-2bcb-3a96-15a2956e7652@baylibre.com> <20170313093630.GE21222@n2100.armlinux.org.uk> <724d599e-7285-3feb-9a18-d3fbc4000eb5@synopsys.com> <20170313131053.GI21222@n2100.armlinux.org.uk> <296108ae-1720-63f1-1eed-ed5dbe1b8bad@synopsys.com> <56e51c49-176e-3316-bd8c-f947061732bf@collabora.com> Cc: Archit Taneja , David Airlie , Heiko Stuebner , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org From: Neil Armstrong Organization: Baylibre Message-ID: <2cdf37b1-810c-b595-4e7f-09353bc3a006@baylibre.com> Date: Tue, 14 Mar 2017 09:13:25 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <56e51c49-176e-3316-bd8c-f947061732bf@collabora.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>>>>> --- >>>>>>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Armstrong Subject: Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions Date: Tue, 14 Mar 2017 09:13:25 +0100 Message-ID: <2cdf37b1-810c-b595-4e7f-09353bc3a006@baylibre.com> References: <20170310093509.19044-1-romain.perier@collabora.com> <25ad96a7-d907-2bcb-3a96-15a2956e7652@baylibre.com> <20170313093630.GE21222@n2100.armlinux.org.uk> <724d599e-7285-3feb-9a18-d3fbc4000eb5@synopsys.com> <20170313131053.GI21222@n2100.armlinux.org.uk> <296108ae-1720-63f1-1eed-ed5dbe1b8bad@synopsys.com> <56e51c49-176e-3316-bd8c-f947061732bf@collabora.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <56e51c49-176e-3316-bd8c-f947061732bf@collabora.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Romain Perier , Jose Abreu , Russell King - ARM Linux Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org List-Id: linux-rockchip.vger.kernel.org T24gMDMvMTQvMjAxNyAwODo1MyBBTSwgUm9tYWluIFBlcmllciB3cm90ZToKPiBIaSwKPiAKPiAK PiBMZSAxMy8wMy8yMDE3IMOgIDE5OjQ5LCBKb3NlIEFicmV1IGEgw6ljcml0IDoKPj4gSGkgUnVz c2VsbCwKPj4KPj4KPj4gT24gMTMtMDMtMjAxNyAxMzoxMCwgUnVzc2VsbCBLaW5nIC0gQVJNIExp bnV4IHdyb3RlOgo+Pj4gT24gTW9uLCBNYXIgMTMsIDIwMTcgYXQgMTI6NTU6NThQTSArMDAwMCwg Sm9zZSBBYnJldSB3cm90ZToKPj4+PiBIaSwKPj4+Pgo+Pj4+Cj4+Pj4gT24gMTMtMDMtMjAxNyAw OTozNiwgUnVzc2VsbCBLaW5nIC0gQVJNIExpbnV4IHdyb3RlOgo+Pj4+PiBPbiBNb24sIE1hciAx MywgMjAxNyBhdCAxMDoyNzowOEFNICswMTAwLCBOZWlsIEFybXN0cm9uZyB3cm90ZToKPj4+Pj4+ IE9uIDAzLzEwLzIwMTcgMTA6MzUgQU0sIFJvbWFpbiBQZXJpZXIgd3JvdGU6Cj4+Pj4+Pj4gQ3Vy cmVudGx5LCB0aGUgYXVkaW8gc2FtcGxlciBjbG9jayBpcyBlbmFibGVkIGZyb20gZHdfaGRtaV9z ZXR1cCgpIGF0Cj4+Pj4+Pj4gc3RlcCBFLiBhbmQgaXMga2VwdCBlbmFibGVkIGZvciBsYXRlciB1 c2UuIFRoaXMgY2xvY2sgc2hvdWxkIGJlIGVuYWJsZWQKPj4+Pj4+PiBhbmQgZGlzYWJsZWQgYWxv bmcgd2l0aCB0aGUgYWN0dWFsIGF1ZGlvIHN0cmVhbSBhbmQgbm90IGFsd2F5cyBvbiAodGhhdAo+ Pj4+Pj4+IGlzIGJhZCBmb3IgUE0pLiBGdXRoZXJtb3JlLCB0aGlzIG1pZ2h0IGNhdXNlIHNvdW5k IGdsaXRjaGVzIHdpdGggc29tZQo+Pj4+Pj4+IEhETUkgZGV2aWNlcywgYXMgdGhlIENUUytOIGlz IGZvcmNlZCB0byB6ZXJvIHdoZW4gdGhlIHN0cmVhbSBpcyBkaXNhYmxlZAo+Pj4+Pj4+IHdoaWxl IHRoZSBhdWRpbyBjbG9jayBpcyBzdGlsbCBydW5uaW5nLgo+Pj4+Pj4+Cj4+Pj4+Pj4gVGhpcyBj b21taXQgYWRkcyBhIHBhcmFtZXRlciB0byBoZG1pX2F1ZGlvX2VuYWJsZV9jbGsoKSB0aGF0IGNv bnRyb2xzCj4+Pj4+Pj4gd2hlbiB0aGUgYXVkaW8gc2FtcGxlIGNsb2NrIG11c3QgYmUgZW5hYmxl ZCBvciBkaXNhYmxlZC4gVGhlbiwgaXQgbW92ZXMKPj4+Pj4+PiB0aGUgY2FsbCB0byB0aGlzIGZ1 bmN0aW9uIGludG8gZHdfaGRtaV9hdWRpb19lbmFibGUoKSBhbmQKPj4+Pj4+PiBkd19oZG1pX2F1 ZGlvX2Rpc2FibGUoKS4KPj4+Pj4+Pgo+Pj4+Pj4+IFNpZ25lZC1vZmYtYnk6IFJvbWFpbiBQZXJp ZXIgPHJvbWFpbi5wZXJpZXJAY29sbGFib3JhLmNvbT4KPj4+Pj4+PiAtLS0KPj4+Pj4+PiAgZHJp dmVycy9ncHUvZHJtL2JyaWRnZS9kdy1oZG1pLmMgfCAxNSArKysrKysrKystLS0tLS0KPj4+Pj4+ PiAgMSBmaWxlIGNoYW5nZWQsIDkgaW5zZXJ0aW9ucygrKSwgNiBkZWxldGlvbnMoLSkKPj4+Pj4+ Pgo+Pj4+Pj4gSGkgUm9tYWluLCBSdXNzZWxsLCBKb3NlLAo+Pj4+Pj4KPj4+Pj4+IFRoaXMgaXMg YSBsaXR0bGUgb3V0IG9mIHNjb3BlLCBidXQgSSB3YXMgd29uZGVyaW5nIHdoeSB0aGUgQ1RTIGNh bGN1bGF0aW9uCj4+Pj4+PiB3YXMgbm90IGxlZnQgaW4gQVVUTyBtb2RlIGluIHRoZSBkdy1oZG1p IGRyaXZlciA/Cj4+Pj4+IFRoZXJlIGlzIG5vIGluZGljYXRpb24gaW4gdGhlIGlNWDYgbWFudWFs cyB0aGF0IHRoZSBpTVg2IHN1cHBvcnRzCj4+Pj4+IGF1dG9tYXRpYyBDVFMgY2FsY3VsYXRpb24u ICBCaXRzIDc6NCBvZiB0aGUgQVVEX0NUUzMgcmVnaXN0ZXIgYXJlCj4+Pj4+IG1hcmtlZCBhcyAi cmVzZXJ2ZWQiLgo+Pj4+Pgo+Pj4+PiBXZSdyZSByZWxpYW50IG9uIHRoZSBpbmZvcm1hdGlvbiBp biB0aGUgaU1YNiBtYW51YWxzIGFzIHdlIGRvbid0IGhhdmUKPj4+Pj4gYWNjZXNzIHRvIFN5bm9w c2lzJyBkYXRhYm9va3MgZm9yIHRoZXNlIHBhcnRzIChJIHVuZGVyc3RhbmQgeW91IGhhdmUKPj4+ Pj4gdG8gYmUgYSBjdXN0b21lciB0byBoYXZlIGFjY2VzcyB0byB0aGF0LikKPj4+Pj4KPj4+PiAo U3lub3BzaXMgLT4gU3lub3BzeXMgOikpCj4+Pj4KPj4+PiBUcnlpbmcgdG8gY2F0Y2ggdXAgd2l0 aCB0aGUgY29udmVyc2F0aW9uOgo+Pj4+Cj4+Pj4gMSkgSW4gQUhCIGF1ZGlvIG1vZGUgdGhlIGJp dHMgYXJlIGFsd2F5cyByZXNlcnZlZC4KPj4+PiAyKSBJIHRoaW5rIHdlIHNob3VsZCBlbmFibGUv ZGlzYWJsZSBjbG9jayBpbnN0ZWFkIG9mIGp1c3QgZm9yY2luZwo+Pj4+IE4vQ1RTLCB0aG91Z2gs IEkgZG9uJ3Qga25vdyB3aGF0IGNvdWxkIGJlIHRoZSBpbXBsaWNhdGlvbnMgZm9yCj4+Pj4gaU1Y IHBsYXRmb3JtLiBJIHJlbWVtYmVyIGF0IHRoZSB0aW1lIEkgdGVzdGVkIHRoaXMgdXNpbmcgSTJT Cj4+Pj4gKEkndmUgbmV2ZXIgdXNlZCBBSEIpLCBhbmQgSERNSSBwcm90b2NvbCBhbmFseXplcnMg d2VyZQo+Pj4+IGNvbXBsYWluaW5nIGFib3V0IHRoZSBOL0NUUyBiZWluZyBmb3JjZWQgdG8gemVy by4KPj4+IFdoYXQgeW91J3JlIHJlY29tbWVuZGluZyBpcyB0byBiYXNpY2FsbHkgaWdub3JlIHRo ZSBwdWJsaXNoZWQgd29ya2Fyb3VuZCwKPj4+IHdoaWNoLCBwcmVzdW1hYmx5LCB3YXMgYXJyaXZl ZCBhdCBieSBwcm9wZXIgYW5hbHlzaXMgb2YgdGhlIGlzc3VlIGJvdGgKPj4+IGJ5IEZyZWVzY2Fs ZSBlbmdpbmVlcnMgYW5kIFN5bm9wc3lzIGVuZ2luZWVycywgYW5kIGluc3RlYWQgdHJ5IHNvbWUg b3RoZXIKPj4+IHNvbHV0aW9uLgo+Pj4KPj4+IEknbSBub3Qgc3VyZSB0aGF0J3MgYSBnb29kIGlk ZWEuICBPbmx5IHRob3NlIHdpdGgga25vd2xlZGdlIG9mIHRoZSBJUCBjYW4KPj4+IHNheSB3aGF0 IGVmZmVjdCBkaXNhYmxpbmcgdGhlIGF1ZGlvIGNsb2NrIHdpbGwgaGF2ZTogd2lsbCBpdCBzdGls bCBhbGxvdwo+Pj4gdGhlIEZJRk8gdG8gYmUgbG9hZGVkLCBhcyByZXF1aXJlZCBieSB0aGUgZXJy YXRhPyAgSWYgbm90LCBpdCdzIG5vdCBhCj4+PiBzb2x1dGlvbiB0aGF0IEFIQiBjYW4gdXNlLiAg SSB3b3VsZCBpbWFnaW5lIHRoYXQgX2lmXyBpdCB3YXMgYXMgc2ltcGxlIGFzCj4+PiBkaXNhYmxp bmcgdGhlIGF1ZGlvIGNsb2NrLCB0aGF0IHNpbXBsZSBhcHByb2FjaCB3b3VsZCBoYXZlIGJlZW4g ZG9jdW1lbnRlZAo+Pj4gaW4gRnJlZXNjYWxlJ3MgZXJyYXRhIGRvY3VtZW50cywgcmF0aGVyIHRo YW4gdGhlIHJhdGhlciBtb3JlIGNvbXBsZXgKPj4+IG1ldGhvZCBvZiB6ZXJvaW5nIHRoZSBDVFMv TiB2YWx1ZXMuCj4+IEknbSBqdXN0IGZvbGxvd2luZyB3aGF0IHRoZSB1c2VyIGd1aWRlIHNheXM6 IHRoZSBmaW5hbCBzdGVwIG9mCj4+IGNvbmZpZ3VyYXRpb24gaXMgZW5hYmxpbmcgdGhlIGF1ZGlv IGNsb2NrLiBUaGVyZSBpcyBubyByZWZlcmVuY2UKPj4gbmVpdGhlciBpbiBkYXRhc2hlZXQgbmVp dGhlciBpbiB1c2VyIGd1aWRlIGFib3V0IHRoaXMgYmVoYXZpb3IKPj4gYnV0LCBhcyBJIHNhaWQs IEkndmUgbmV2ZXIgdXNlZCBBSEIgc28gSSBjYW4ndCBwcm92ZSB3aGF0IGlzIHRoZQo+PiBiZXN0 IHNvbHV0aW9uLiBDb3VsZCBpdCBiZSBwbGF0Zm9ybSBzcGVjaWZpYz8KPiAKPiBBbmQgdGhhdCdz IHByZWNpc2VseSB3aHkgSSBhbSBlbmFibGluZyBpdAo+IAo+Pgo+Pj4gSSB0aGluayB0aGVyZSBh cmUgdHdvIGNob2ljZXMgaGVyZToKPj4+Cj4+PiAxKSBnZXQgc29tZSBkZWZpbml0aXZlIGluZm9y bWF0aW9uIGFib3V0IHRoZSBJUCBhbmQgdGhlIGVycmF0YSBmb3IgQUhCLAo+Pj4gICAgYW5kIGRl dGVybWluZSB3aGV0aGVyIHRoZSBzb2x1dGlvbiB5b3UgcHJvcG9zZSB3b3VsZCB3b3JrLiAgKFRo YXQncwo+Pj4gICAgb3V0IG9mIHNjb3BlIGZvciBtZSwgSSd2ZSBubyBjb250YWN0cyB3aXRoIEZT TC9OWFAgYW5kIEknbSBub3QgYQo+Pj4gICAgU3lub3BzeXMgY3VzdG9tZXIuKQo+PiBUaGlzIGlz IHRoZSByaWdodCB0aGluZyB0byBkbyBidXQgaWYgeW91IGNhbid0IHRlc3QgaW4gdGhlCj4+IEZy ZWVzY2FsZSBwbGF0Zm9ybSB0aGVuIHdlIGhhdmUgbm90IG11Y2ggZWxzZSB0byBkby4KPj4KPj4g QmVzdCByZWdhcmRzLAo+PiBKb3NlIE1pZ3VlbCBBYnJldQo+Pgo+Pj4gMikgdGhlIEkyUyBhdWRp byBzdXBwb3J0IHN0b3BzIHJlLXVzaW5nIHRoZSBBSEIgYXVkaW8gc3VwcG9ydCBmdW5jdGlvbnMs Cj4+PiAgICBzd2l0Y2hpbmcgdG8gdGhlaXIgb3duIGltcGxlbWVudGF0aW9uIHRoYXQgYmVoYXZl cyBjb3JyZWN0bHkgZm9yIHRoZW0uCj4+PiAgICAoUmVtZW1iZXIsIGl0IHdhcyBJMlMncyBjaG9p Y2UgdG8gcmUtdXNlIHRoZSBBSEIgYXVkaW8gc3VwcG9ydCBmdW5jdGlvbnMKPj4+ICAgIEkgYWRk ZWQgd2hpY2ggd2UncmUgbm93IGRpc2N1c3NpbmcgLSB0aGV5IGRpZG4ndCBoYXZlIHRvIGRvIHRo YXQsIGFuZAo+Pj4gICAgcmVncmVzc2luZyBBSEIgYXVkaW8ganVzdCBmb3IgSTJTIGlzIG5vdCBz b21ldGhpbmcgd2Ugc2hvdWxkIGV2ZXIKPj4+ICAgIGNvbnNpZGVyIGRvaW5nLikKPj4+Cj4gCj4g VGhlIHdvcmthcm91bmQgbG9va3MgQUhCIHNwZWNpZmljIGluIGFueSBjYXNlcyBhbmQgZG9lcyBu b3Qgc2VlbSB0byB3b3JrCj4gd2l0aCBJMlMuIFRoZSBJMlMgdmFyaWFudCBzaG91bGQgbm90IHVz ZWQgdGhlIHNhbWUgZnVuY3Rpb25zLCBhdCBsZWFzdAo+IGZvciBlbmFibGluZy9kaXNhYmxpbmcg YXVkaW8gc3RyZWFtLgo+IAo+IFJlZ2FyZHMsCj4gUm9tYWluCj4gCkhpIEFsbCwKCkpvc2UsIHRo YW5rcyBmb3IgeW91ciBjbGFyaWZpY2F0aW9uLgoKSXQgc2VlbXMgcmVhc29uYWJsZSB0byBpbmRl ZWQgYWRkIEkyUy9TUERJRiBzcGVjaWZpYyBmdW5jdGlvbnMsIG9yIGF0IGxlYXN0IGFkZApwYXJh bWV0ZXJzIHRvIGNoYW5nZSB0aGUgYmVoYXZpb3Igd2hlbiB0aGUgYXVkaW8gY2xvY2sgaXMgZ2Vu ZXJhdGVkIGJ5IHRoZQpTeW5vcHN5cyBJUCBvciBieSBhbiBleHRlcm5hbCBhdWRpbyBibG9jayBh cyBpbiBJMlMgYW5kIFNQRElGLgoKTmV2ZXJ0aGVsZXNzLCBvbiB0aGUgQW1sb2dpYyBwbGF0Zm9y bXMsIHdlIHdpbGwgbmVlZCB0byBhZGQgdGhpcyBBVVRPIGN0cyBjYWxjdWxhdGlvbgpmZWF0dXJl IGFuZCB3aWxsIGNlcnRhaW5seSBuZWVkIHRoaXMgYmVoYXZpb3IgZml4LgoKUm9tYWluLCBmZWVs IGZyZWUgdG8gYWRkIHRoZSBjb3JyZWN0IEkyUyBiZWhhdmlvciwgSSdsbCBiZSBoYXBweSB0byB0 ZXN0IHlvdXIgcGF0Y2hlcy4KClRoYW5rcywKTmVpbApfX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBs aXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1h bi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: narmstrong@baylibre.com (Neil Armstrong) Date: Tue, 14 Mar 2017 09:13:25 +0100 Subject: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions In-Reply-To: <56e51c49-176e-3316-bd8c-f947061732bf@collabora.com> References: <20170310093509.19044-1-romain.perier@collabora.com> <25ad96a7-d907-2bcb-3a96-15a2956e7652@baylibre.com> <20170313093630.GE21222@n2100.armlinux.org.uk> <724d599e-7285-3feb-9a18-d3fbc4000eb5@synopsys.com> <20170313131053.GI21222@n2100.armlinux.org.uk> <296108ae-1720-63f1-1eed-ed5dbe1b8bad@synopsys.com> <56e51c49-176e-3316-bd8c-f947061732bf@collabora.com> Message-ID: <2cdf37b1-810c-b595-4e7f-09353bc3a006@baylibre.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 >>>>>>> --- >>>>>>> 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