From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Looijmans Subject: Re: [PATCH] sound/soc/adi/axi-spdif.c: Support programmable master clock Date: Thu, 4 Dec 2014 15:18:54 +0100 Message-ID: <54806D4E.8040504@topic.nl> References: <1417675940-3754-1-git-send-email-mike.looijmans@topic.nl> <5480577D.4010106@metafoo.de> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: Received: from smtp103.mer-nm.internl.net (smtp103.mer-nm.internl.net [217.149.192.139]) by alsa0.perex.cz (Postfix) with ESMTP id 83CA726053E for ; Thu, 4 Dec 2014 15:19:00 +0100 (CET) In-Reply-To: <5480577D.4010106@metafoo.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Lars-Peter Clausen Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org List-Id: alsa-devel@alsa-project.org 77u/T24gMTIvMDQvMjAxNCAwMTo0NSBQTSwgTGFycy1QZXRlciBDbGF1c2VuIHdyb3RlOgo+IE9u IDEyLzA0LzIwMTQgMDc6NTIgQU0sIE1pa2UgTG9vaWptYW5zIHdyb3RlOgo+PiBJZiB0aGUgbWFz dGVyIGNsb2NrIHN1cHBvcnRzIHByb2dyYW1tYWJsZSByYXRlcywgcHJvZ3JhbSBpdCB0byBnZW5l cmF0ZQo+PiB0aGUgZGVzaXJlZCBmcmVxdWVuY3kuIE9ubHkgYXBwbHkgY29uc3RyYWludHMgd2hl biB0aGUgY2xvY2sgaXMgZml4ZWQuCj4+IFRoaXMgYWxsb3dzIHByb3BlciBjbG9jayBnZW5lcmF0 aW9uIGZvciBib3RoIDQ0MTAwIGFuZCA0ODAwMCBIeiBiYXNlZAo+PiBzYW1wbGluZyByYXRlcyBp ZiB0aGUgcGxhdGZvcm0gc3VwcG9ydHMgaXQuCj4+Cj4+IFRoZSBjbG9jayBmcmVxdWVuY3kgbXVz dCBiZSBzZXQgYmVmb3JlIGVuYWJsaW5nIGl0LiBFbmFibGluZyB0aGUgY2xvY2sKPj4gd2FzIGRv bmUgaW4gInN0YXJ0dXAiLCBidXQgdGhhdCBvY2N1cnMgYmVmb3JlICJod19wYXJhbXMiIHdoZXJl IHRoZSByYXRlCj4+IGlzIGtub3duLiBNb3ZlIHRoZSBjbG9jayBzdGFydCB0byB0aGUgaHdfcGFy YW1zIHJvdXRpbmUsIGFuZCBrZWVwIHRyYWNrCj4+IG9mIHdoZXRoZXIgdGhlIGNsb2NrIGhhcyBi ZWVuIHN0YXJ0ZWQsIGJlY2F1c2Ugc2h1dGRvd24gbWF5IGJlIGNhbGxlZAo+PiB3aXRob3V0IGhh dmluZyBjYWxsZWQgaHdfcGFyYW1zIGZpcnN0Lgo+Cj4gVXN1YWxseSB0aGF0IHNob3VsZG4ndCBi ZSBhIHByb2JsZW0uIElmIHlvdXIgY2xvY2sgY2hpcCByZXF1aXJlcyBpdCB0byBiZQo+IGRpc2Fi bGVkIGluIG9yZGVyIHRvIGJlIHJlcHJvZ3JhbW1lZCB0aGFuIHRoZSBDTEtfU0VUX1JBVEVfR0FU RSBmbGFnIHNob3VsZCBiZQo+IHNldC4gVGhpcyB3aWxsIHRlbGwgdGhlIGNvcmUgdG8gZGlzYWJs ZSB0aGUgY2xvY2sgYmVmb3JlIGNoYW5naW5nIGl0LgoKVGhlIGlzc3VlIGhlcmUgaXMgbm90IHRo ZSBjbG9jaydzIGNhcGFiaWxpdGllcywgYnV0IHRoZSBvcmRlciBpbiB3aGljaCB0aGluZ3MgCmFy ZSBiZWluZyBkb25lLiBJZiB0aGUgZHJpdmVyIGp1c3QgZW5hYmxlcyB0aGUgY2xvY2sgd2l0aG91 dCBldmVyIGhhdmluZyBzZXQgYSAKcmF0ZSwgdGhlIGNsb2NrIHdpbGwgcnVuIGF0IHdoYXRldmVy IGhhcHBlbnMgdG8gYmUgdGhlIGRlZmF1bHQuIFRoYXQgZGVmYXVsdCAKbWF5IGhhdmUgdW5wcmVk aWN0YWJsZSByZXN1bHRzIG9yIGV2ZW4gYmUgaGFybWZ1bCB0byB0aGUgc3lzdGVtLiBTbyB0aGUg ZHJpdmVyIApzaG91bGQgZmlyc3Qgc2V0IGEgdmFsaWQgY2xvY2sgcmF0ZSBiZWZvcmUgZW5hYmxp bmcgdGhlIGNsb2NrLiBTdWdnZXN0aW9ucyBvbiAKcmV3b3JkaW5nIG15IGNvbW1lbnRzIHRvIGJl dHRlciByZWZsZWN0IHRoYXQgYXJlIHdlbGNvbWUuCgo+Cj4gWy4uLl0KPj4gICBzdGF0aWMgY29u c3Qgc3RydWN0IHNuZF9zb2NfZGFpX29wcyBheGlfc3BkaWZfZGFpX29wcyA9IHsKPj4gQEAgLTIx NiwxNCArMjI3LDE3IEBAIHN0YXRpYyBpbnQgYXhpX3NwZGlmX3Byb2JlKHN0cnVjdCBwbGF0Zm9y bV9kZXZpY2UgKnBkZXYpCj4+ICAgICAgIHNwZGlmLT5kbWFfZGF0YS5hZGRyX3dpZHRoID0gNDsK Pj4gICAgICAgc3BkaWYtPmRtYV9kYXRhLm1heGJ1cnN0ID0gMTsKPj4KPj4gLSAgICBzcGRpZi0+ cmF0bnVtLm51bSA9IGNsa19nZXRfcmF0ZShzcGRpZi0+Y2xrX3JlZikgLyAxMjg7Cj4+IC0gICAg c3BkaWYtPnJhdG51bS5kZW5fc3RlcCA9IDE7Cj4+IC0gICAgc3BkaWYtPnJhdG51bS5kZW5fbWlu ID0gMTsKPj4gLSAgICBzcGRpZi0+cmF0bnVtLmRlbl9tYXggPSA2NDsKPj4gLQo+PiAtICAgIHNw ZGlmLT5yYXRlX2NvbnN0cmFpbnRzLnJhdHMgPSAmc3BkaWYtPnJhdG51bTsKPj4gLSAgICBzcGRp Zi0+cmF0ZV9jb25zdHJhaW50cy5ucmF0cyA9IDE7Cj4+ICsgICAgLyogRGV0ZXJtaW5lIGlmIHRo ZSBjbG9jayByYXRlIGlzIGZpeGVkLiBJZiBpdCBjYW5ub3QgY2hhbmdlIGZyZXF1ZW5jeSwKPj4g KyAgICAgKiBpdCByZXR1cm5zIGFuIGVycm9yIGhlcmUuICovCj4+ICsgICAgaWYgKGNsa19yb3Vu ZF9yYXRlKHNwZGlmLT5jbGtfcmVmLCAxMjggKiA0NDEwMCkgPCAwKSB7Cj4KPiBJIGRvbid0IHRo aW5rIHRoaXMgd29ya3MuIEZvciBhIGZpeGVkIGNsb2NrIGNsa19yb3VuZF9yYXRlKCkgd2lsbCBy ZXR1cm4gdGhlCj4gZml4ZWQgcmF0ZSByYXRoZXIgdGhhbiBhbiBlcnJvci4gSSB0cmllZCB0aGUg cGF0Y2ggYW5kIGV2ZW4gdGhvdWdoIEkgaGF2ZSBhCj4gZml4ZWQgY2xvY2sgdGhlIGNvbnN0cmFp bnRzIGFyZSBubyBsb25nZXIgc2V0Lgo+Cj4gVGhlcmUgaXMgdW5mb3J0dW5hdGVseSBubyBnb29k IHdheSB0byBlbnVtZXJhdGUgd2hpY2ggZnJlcXVlbmNpZXMgYXJlCj4gc3VwcG9ydGVkIGJ5IGEg Y2xvY2sgb3RoZXIgdGhhbiBqdXN0IGNhbGxpbmcgcm91bmRfcmF0ZSBmb3IgYWxsIHBvc3NpYmxl IHJhdGVzLgo+Cj4gSSB0aGluayB0aGUgYmVzdCB3YXkgdG8gaW1wbGVtZW50IHRoaXMgZm9yIG5v dyBpcyB0byB0cnkgZS5nLiAzMjAwMCAqIDEyOCwKPiA0NDEwMCAqIDEyOCwgNDgwMDAgKiAxMjgg YW5kIHRoZW4gY2hlY2sgaWYgY2xrX3JvdW5kX3JhdGUgcmV0dXJucyB0aGUgZXhwZWN0ZWQKPiBy YXRlIGFuZCBpZiBpdCBkb2VzIHNldCB1cCBhIHJhdGUgY29uc3RyYWludCBmb3IgdGhhdCByYXRl LgoKRm9yIHdoYXQgSSBjb3VsZCBzZWUsIEFMU0EgbmV2ZXIgcmVwb3J0ZWQgb3IgbGltaXRlZCB0 aGUgc2FtcGxlIHJhdGUgY29ycmVjdGx5IApldmVuIGJlZm9yZSB0aGlzIHBhdGNoLCBzbyBtYXli ZSB0aGUgZXZlbiBzaW1wbGVyIGFwcHJvYWNoIGlzIGJldHRlcjogSnVzdCAKcmVtb3ZlIHRoZSBj b25zdHJhaW50LiBJIHdvbmRlciBob3cgeW91IGNvbmNsdWRlZCB0aGF0IHRoZSBjb25zdHJhaW50 IGRpZG4ndCAKZ2V0IGFkZGVkPwoKQWN0dWFsbHksIHRoZSBTUERJRiBsb2dpYyB3YW50cyBhIGNs b2NrIHRoYXQgaXMgYW4gImludGVnZXIgbXVsdGlwbGUgb2YgCjEyOCpzYW1wbGVyYXRlIGluIHRo ZSByYW5nZSBvZiAxLi42NCIuIE90aGVyIHRoYW4ganVzdCBsb29waW5nIHRocm91Z2ggdGhlbSAK YWxsLCB0aGVyZSdzIGFsc28gbm8gd2F5IHRvIHJlcXVlc3QgdGhlIGNsb2NrIGZyYW1ld29yayBm b3Igc3VjaCBhIHJlcXVpcmVtZW50LgoKWW91ciBzdWdnZXN0aW9uIGlzIHF1aXRlIHJvYnVzdCB0 aG91Z2gsIGJ1dCB0aGUgZml4ZWQgY2xvY2sgbWF5IGJlIHNldCB0byBhbnkgCmludGVnZXIgbXVs dGlwbGUgb2YgdGhlIGRlc2lyZWQgZnJlcXVlbmN5LCBzbyB0aGUgYWxnb3JpdGhtIHdvdWxkIG5v dCB3b3JrIGlmIAp0aGUgZml4ZWQgY2xvY2sgaXMgcnVubmluZyBhdCA0ODAwMCoyNTYgKDEyLjhN SHogbGlrZSBpbiB0aGUgcmVmZXJlbmNlIApkZXNpZ24pLiBTbyBJIGd1ZXNzIHRoZSBiZXN0IEkg Y291bGQgZG8gaGVyZSBpcyBqdXN0IGNoZWNrIHRoYXQ6CgpjbGtfcm91bmRfcmF0ZShzcGRpZi0+ Y2xrX3JlZiwgMTI4ICogNDQxMDApICE9IGNsa19yb3VuZF9yYXRlKHNwZGlmLT5jbGtfcmVmLCAK MTI4ICogNDgwMDApCgpPbmx5IGEgY29uZmlndXJhYmxlIGNsb2NrIHRoYXQgaXMgY29tcGF0aWJs ZSB3aXRoIHRoZSBwYXRjaCB3b3VsZCBkbyB0aGF0LgoKCgoKTWV0IHZyaWVuZGVsaWprZSBncm9l dCAvIGtpbmQgcmVnYXJkcywKCk1pa2UgTG9vaWptYW5zClN5c3RlbSBFeHBlcnQKCgpUT1BJQyBF bWJlZGRlZCBTeXN0ZW1zCkVpbmRob3ZlbnNld2VnIDMyLUMsIE5MLTU2ODMgS0ggQmVzdApQb3N0 YnVzIDQ0MCwgTkwtNTY4MCBBSyBCZXN0ClRlbGVmb29uOiAoKzMxKSAoMCkgNDk5IDMzIDY5IDc5 ClRlbGVmYXg6ICAoKzMxKSAoMCkgNDk5IDMzIDY5IDcwCkUtbWFpbDogbWlrZS5sb29pam1hbnNA dG9waWMubmwKV2Vic2l0ZTogd3d3LnRvcGljLm5sCgpQbGVhc2UgY29uc2lkZXIgdGhlIGVudmly b25tZW50IGJlZm9yZSBwcmludGluZyB0aGlzIGUtbWFpbAoKVG9waWMgem9la3QgZ2VkcmV2ZW4g KGVtYmVkZGVkKSBzb2Z0d2FyZSBzcGVjaWFsaXN0ZW4hCmh0dHA6Ly90b3BpYy5ubC92YWNhdHVy ZXMvdG9waWMtem9la3Qtc29mdHdhcmUtZW5naW5lZXJzLwoKX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX18KQWxzYS1kZXZlbCBtYWlsaW5nIGxpc3QKQWxzYS1k ZXZlbEBhbHNhLXByb2plY3Qub3JnCmh0dHA6Ly9tYWlsbWFuLmFsc2EtcHJvamVjdC5vcmcvbWFp bG1hbi9saXN0aW5mby9hbHNhLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754505AbaLDOTH (ORCPT ); Thu, 4 Dec 2014 09:19:07 -0500 Received: from smtp103.mer-nm.internl.net ([217.149.192.139]:36704 "EHLO smtp103.mer-nm.internl.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753908AbaLDOTF convert rfc822-to-8bit (ORCPT ); Thu, 4 Dec 2014 09:19:05 -0500 X-Spam-Flag: NO X-Spam-Score: -2.899 X-Spam-Languages: en Message-ID: <54806D4E.8040504@topic.nl> Date: Thu, 4 Dec 2014 15:18:54 +0100 From: Mike Looijmans User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Lars-Peter Clausen CC: , Subject: Re: [PATCH] sound/soc/adi/axi-spdif.c: Support programmable master clock References: <1417675940-3754-1-git-send-email-mike.looijmans@topic.nl> <5480577D.4010106@metafoo.de> In-Reply-To: <5480577D.4010106@metafoo.de> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8BIT X-Originating-IP: [192.168.80.45] X-EXCLAIMER-MD-CONFIG: 9833cda7-5b21-4d34-9a38-8d025ddc3664 X-EXCLAIMER-MD-BIFURCATION-INSTANCE: 0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/04/2014 01:45 PM, Lars-Peter Clausen wrote: > On 12/04/2014 07:52 AM, Mike Looijmans wrote: >> If the master clock supports programmable rates, program it to generate >> the desired frequency. Only apply constraints when the clock is fixed. >> This allows proper clock generation for both 44100 and 48000 Hz based >> sampling rates if the platform supports it. >> >> The clock frequency must be set before enabling it. Enabling the clock >> was done in "startup", but that occurs before "hw_params" where the rate >> is known. Move the clock start to the hw_params routine, and keep track >> of whether the clock has been started, because shutdown may be called >> without having called hw_params first. > > Usually that shouldn't be a problem. If your clock chip requires it to be > disabled in order to be reprogrammed than the CLK_SET_RATE_GATE flag should be > set. This will tell the core to disable the clock before changing it. The issue here is not the clock's capabilities, but the order in which things are being done. If the driver just enables the clock without ever having set a rate, the clock will run at whatever happens to be the default. That default may have unpredictable results or even be harmful to the system. So the driver should first set a valid clock rate before enabling the clock. Suggestions on rewording my comments to better reflect that are welcome. > > [...] >> static const struct snd_soc_dai_ops axi_spdif_dai_ops = { >> @@ -216,14 +227,17 @@ static int axi_spdif_probe(struct platform_device *pdev) >> spdif->dma_data.addr_width = 4; >> spdif->dma_data.maxburst = 1; >> >> - spdif->ratnum.num = clk_get_rate(spdif->clk_ref) / 128; >> - spdif->ratnum.den_step = 1; >> - spdif->ratnum.den_min = 1; >> - spdif->ratnum.den_max = 64; >> - >> - spdif->rate_constraints.rats = &spdif->ratnum; >> - spdif->rate_constraints.nrats = 1; >> + /* Determine if the clock rate is fixed. If it cannot change frequency, >> + * it returns an error here. */ >> + if (clk_round_rate(spdif->clk_ref, 128 * 44100) < 0) { > > I don't think this works. For a fixed clock clk_round_rate() will return the > fixed rate rather than an error. I tried the patch and even though I have a > fixed clock the constraints are no longer set. > > There is unfortunately no good way to enumerate which frequencies are > supported by a clock other than just calling round_rate for all possible rates. > > I think the best way to implement this for now is to try e.g. 32000 * 128, > 44100 * 128, 48000 * 128 and then check if clk_round_rate returns the expected > rate and if it does set up a rate constraint for that rate. For what I could see, ALSA never reported or limited the sample rate correctly even before this patch, so maybe the even simpler approach is better: Just remove the constraint. I wonder how you concluded that the constraint didn't get added? Actually, the SPDIF logic wants a clock that is an "integer multiple of 128*samplerate in the range of 1..64". Other than just looping through them all, there's also no way to request the clock framework for such a requirement. Your suggestion is quite robust though, but the fixed clock may be set to any integer multiple of the desired frequency, so the algorithm would not work if the fixed clock is running at 48000*256 (12.8MHz like in the reference design). So I guess the best I could do here is just check that: clk_round_rate(spdif->clk_ref, 128 * 44100) != clk_round_rate(spdif->clk_ref, 128 * 48000) Only a configurable clock that is compatible with the patch would do that. Met vriendelijke groet / kind regards, Mike Looijmans System Expert TOPIC Embedded Systems Eindhovenseweg 32-C, NL-5683 KH Best Postbus 440, NL-5680 AK Best Telefoon: (+31) (0) 499 33 69 79 Telefax: (+31) (0) 499 33 69 70 E-mail: mike.looijmans@topic.nl Website: www.topic.nl Please consider the environment before printing this e-mail Topic zoekt gedreven (embedded) software specialisten! http://topic.nl/vacatures/topic-zoekt-software-engineers/