On 4/11/2018 11:44 PM, Daniel Kurtz wrote:
On Wed, Apr 11, 2018 at 4:08 AM Agrawal, Akshu Akshu.Agrawal@amd.com wrote:
On 4/10/2018 11:13 AM, Daniel Kurtz wrote:
On 4/4/2018 12:27 AM, Daniel Kurtz wrote:
Hi Akshu,
On Fri, Mar 30, 2018 at 4:05 AM Akshu Agrawal akshu.agrawal@amd.com
wrote:
This enables/disables and sets auxiliary clock at 25Mhz. It uses common clock framework for proper ref counting. This approach will save power in comparison to keeping it always On in firmware.
I have some general concern with the approach in this patch:
(1) The 25 MHz clock being created here is a general system clock resource. I think it should be created in an SoC clock driver, rather
than
here in an audio machine driver. This has several advantages: (a) the clock resource would be available for other boards that
use
it but
don't use this particular audio machine driver (b) the clock would be instantiated at boot, rather than at
machine
driver
load. (c) the machine driver would not then need the additional clkdev
code
(2) The oscout1 clock is a pretty standard clk_gate with a clk_mux.
We
can
use some standard helpers to better model the clock tree rather than rewriting them:
static struct clk *clk_48m; static struct clk *clk_25m; static struct clk *clk_oscout1_mux; static struct clk *clk_oscout1;
static const char *clk_oscout1_parents[] = { "clk48MHz", "clk25MHz" };
{ clk_48m = clk_register_fixed_rate(dev, "clk48MHz", NULL, 0,
48000000);
clk_25m = clk_register_fixed_rate(dev, "clk25MHz", NULL, 0,25000000);
res = platform_get_resource(pdev, IORESOURCE_MEM, 0); res_base = devm_ioremap_nocache(dev, res->start,resource_size(res));
clk_oscout1_mux = clk_register_mux(dev, "oscout1_mux", clk_oscout1_parents, ARRAY_SIZE(clk_oscout1_parents), 0, res_base + CLKDRVSTR2, OSCOUT1CLK25MHZ, 3, 0, NULL); clk_set_parent(clk_oscout1_mux, clk_25m); clk_oscout1 = clk_register_gate(dev, "oscout1", "oscout1_mux",0,
res_base + MISCCLKCNTL1, OSCCLKENB, 0, NULL);(3) IIUC, the 25 MHz clock (oscout1) is actually the da7219's mclk.
The
da7219 already has infrastructure to enable/disable its mclk as needed. Once we
establish
the system clock, we can then wire this clock up to the da7219 via ACPI / devicetree properties.
The above design is something used in almost all ARM based platform where we define all mux, pll, divider and gate and then each device
uses
them though the devicetree. The clock here are controlled in firmware. Enable/disable is handled in firmware, where this being the exception case. Hence, I don't think it will be of much value add.
The value add to properly modeling the SoC's clock tree is highlighted above. In addition to making the code more understandable, and the SoC easier
to
use for other designs in the future, the clock tree as expressed via debugfs (/sys/kernel/debug/clk/clk_summary) will make more sense. In the particular case of the acp-da7219-max98357a.c machine driver and da7219 codec driver, properly modeling the oscout1 clock tree as a fixed_clks+mux+gate and then passing it to da7219 as its mclk will allow the mclk driver to do what it expects using its existing code - set the mclk rate (via oscout1's parent mux selector) and then enable/disable as needed.
For ST/CZ we have other platforms which do not use this clock. They
have
a dedicated Oscillator for the codec.
Exactly, such platforms will just not use this machine driver and this clock will be defined properly, but remain in the state programmed by firmware.
If you think its beneficial for other platform based on ST/CZ then
maybe
we can take this as a separate activity. Where we move the clocks to a common file and use the same framework.
Moving the clock configuration out of acp-da7219-max98357a.c allows this system clock to be available independent of when this machine driver
kernel
module is loaded.
I understand the advantages of having it moved to drivers/clk and also connecting it to d7219's mclk. This is good to have feature for other ST/CZ platforms. Would take it up this as a separate activity.
I don't think this is a separate activity. It is an integral part of the structure of this patch. Let's fix it up now while we are actively thinking about this driver.
Ok, then will first move the clk to clock tree framework.
Enable on hw_params() and disable on hw_free() doesn't sound balanced? Will there always be exactly one hw_free() for every hw_params()?
Yes, all resources allocated by hw_paramns are freed in hw_free.
Right, but is it possible for hw_params() to get called mulitple times *without* hw_free()? This would cause more enables than disables.
Documentation/sound/kernel-api/writing-an-alsa-driver.rst says: """ Note that this and ``prepare`` callbacks may be called multiple times per initialization. For example, the OSS emulation may call these callbacks at each change via its ioctl.
Thus, you need to be careful not to allocate the same buffers many times, which will lead to memory leaks! Calling the helper function above many times is OK. It will release the previous buffer automatically when it was already allocated. """
I get there is a remote possibility that hw_params maybe called multiple times without hw_free (though I haven't come across it in my testing). The other option was to use trigger. But, that is not possible as kernel crashes when we call clk_prepare_enable(da7219_dai_clk) from trigger registered to a non da7219 codec, like of max98357
Using trigger won't work because you are not guaranteed to have symetric start/stop events. In fact , there is a comment in the definition of struct snd_soc_dai_ops that specifically says this:
/*
- NOTE: Commands passed to the trigger function are not necessarily
- compatible with the current state of the dai. For example this
- sequence of commands is possible: START STOP STOP.
- So do not unconditionally use refcounting functions in the trigger
- function, e.g. clk_enable/disable.
*/ int (*trigger)(struct snd_pcm_substream *, int, struct snd_soc_dai *);
Instead, can you try to use startup() / shutdown()? Those callbacks are non-atomic and should correspond to open()/close() on the corresponding audio device, which I think is what we want.
ok, startup/shutdown looks good will move to that. But, it won't be as aggressive.
-Dan
Thanks, Akshu