[alsa-devel] [PATCH 2/2] ASoC: AMD: Enable/Disable auxiliary clock via common clock framework

[Daniel Kurtz]

> 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 at 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.

[snip]

> >> +struct cz_clock {
> >> +       const char* acp_clks_name;
> >
> > I still think you should remove acp_clks_name, acp_clks_lookup and
acp_clks
> > from the struct since they are used locally during
> > register_acpd7219_clocks, and, afaict, never referenced again.
> >
> I don't think putting in struct is bad. Plus it helps me get res_base
> value on clk_enable using clk_hw.

Yes, you can use a struct to store the fields that are needed, like
res_base.  However, fields that are only used locally in a single function
waste memory and confuse driver reviewers, since storing them in the struct
implies that the field is state that must be preserved, when in fact the
field's use is transitory.

Please remove the extraneous fields from the struct: acp_clk_name,
acp_clk_lookup, and acp_clk (unless you write an unregister that releases
this clock, and therefore needs this).

> +struct cz_clock {
> +     const char* acp_clk_name;
> +     struct clk_hw acp_clk_hw;
> +     struct clk_lookup *acp_clk_lookup;
> +     struct clk *acp_clk;
> +     void __iomem *res_base;
> +};

[snip]

> >
> > 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.
"""

[snip]

> >> +static int register_acpd7219_clocks(struct platform_device *pdev)
> >
> > Where is the corresponding unregister?  Isn't this a module that can be
> > unloaded?
> >
> I see none of the machine driver have registered a remove callback, I
> wonder why?

I don't know, either?  Perhaps because they don't instantiate anything that
won't be cleaned up by devm_ on unload?
This patch as written is currently instantiating clocks, though, so should
really clean that up when removed.

-Dan