[alsa-devel] [PATCH 2/2] ASoC: AMD: Enable/Disable auxiliary clock via common clock framework
Daniel Kurtz
djkurtz at chromium.org
Tue Apr 3 20:57:51 CEST 2018
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.
(4) Instead of a adding inline "#ifdef CONFIG_COMMON_CLK", can we just make
this driver select COMMON_CLK in its Kconfig instead? When would you use
this machine driver without COMMON_CLK?
Below are additional code review comments on the current version of the
patch, which may be obsolete if we change approach...
> TEST= aplay -vv <file>
> check register to see clock enabled
> kill aplay
> check register to see clock disabled
> Signed-off-by: Akshu Agrawal <akshu.agrawal at amd.com>
> Acked-by: Alex Deucher <alexander.deucher at amd.com>
> ---
> V2: Correcting the pin to OSCOUT1 from OSCOUT2
> V3: Fix error/warnings from kbuild test
> V4: Fix build errors for platform which do not support CONFIG_COMMON_CLK
> V5: Review comments by Dan and Sriram
> sound/soc/amd/acp-da7219-max98357a.c | 159
++++++++++++++++++++++++++++++++++-
> 1 file changed, 157 insertions(+), 2 deletions(-)
> diff --git a/sound/soc/amd/acp-da7219-max98357a.c
b/sound/soc/amd/acp-da7219-max98357a.c
> index d9491e1..9e649c3 100644
> --- a/sound/soc/amd/acp-da7219-max98357a.c
> +++ b/sound/soc/amd/acp-da7219-max98357a.c
> @@ -30,19 +30,41 @@
> #include <sound/soc-dapm.h>
> #include <sound/jack.h>
> #include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> #include <linux/gpio.h>
> #include <linux/module.h>
> #include <linux/i2c.h>
> #include <linux/input.h>
> #include <linux/acpi.h>
> +#include <linux/types.h>
> #include "../codecs/da7219.h"
> #include "../codecs/da7219-aad.h"
> -#define CZ_PLAT_CLK 24000000
> +#define CZ_PLAT_CLK 25000000
> #define MCLK_RATE 24576000
IIUC, "MCLK_RATE" here is misnamed. "MCLK" is the input clock to the
DA7219, that is, it is the 25 MHz "CZ_PLAT_CLK" being generated on OSCOUT1
and sent to the mclk pin of the da7219. The rate here (24.576 MHz) is
actually the clock rate of the DA7219's internal VCO (FVCO), which will be
then be divided down by the DA7219's PLL to give the output BCLK.
> #define DUAL_CHANNEL 2
> +/* Clock Driving Strength 2 register */
> +#define CLKDRVSTR2 0x28
> +/* Clock Control 1 register */
> +#define MISCCLKCNTL1 0x40
> +/* Auxiliary clock1 enable bit */
> +#define OSCCLKENB 2
> +/* 25Mhz auxiliary output clock freq bit */
> +#define OSCOUT1CLK25MHZ 16
> +
> +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.
Also why the 's' in "clks"? It looks like there is just one clock, so just
use the singular, acp_clk.
> +#ifdef CONFIG_COMMON_CLK
> + struct clk_hw acp_clks_hw;
> +#endif
> + struct clk_lookup *acp_clks_lookup;
> + struct clk *acp_clks;
> + void __iomem *res_base;
> +};
> +
> static struct snd_soc_jack cz_jack;
> struct clk *da7219_dai_clk;
> @@ -98,6 +120,8 @@ static int cz_da7219_hw_params(struct
snd_pcm_substream *substream,
> {
> int ret = 0;
> struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_card *card = rtd->card;
> + struct clk *acpd7219_clk;
> ret = clk_prepare_enable(da7219_dai_clk);
> if (ret < 0) {
> @@ -105,13 +129,37 @@ static int cz_da7219_hw_params(struct
snd_pcm_substream *substream,
> return ret;
> }
> + acpd7219_clk = clk_get(card->dev, "acpd7219-clks");
Does clk_get take a reference? Should there be a corresponding clk_put?
Can we instead avoid doing this on every hw_params/hw_free and rather do a
single devm_clk_get() during probe?
> + if (IS_ERR(acpd7219_clk)) {
> + dev_err(rtd->dev, "failed to get clock: %ld\n",
> + PTR_ERR(acpd7219_clk));
> + return PTR_ERR(acpd7219_clk);
> + }
> +
> + ret = clk_prepare_enable(acpd7219_clk);
> + if (ret < 0)
> + dev_err(rtd->dev, "can't enable oscillator clock %d\n",
ret);
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()?
> return ret;
> }
> static int cz_da7219_hw_free(struct snd_pcm_substream *substream)
> {
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_card *card = rtd->card;
> + struct clk *acpd7219_clk;
> +
> clk_disable_unprepare(da7219_dai_clk);
> + acpd7219_clk = clk_get(card->dev, "acpd7219-clks");
> + if (IS_ERR(acpd7219_clk)) {
> + dev_err(rtd->dev, "failed to get clock: %ld\n",
> + PTR_ERR(acpd7219_clk));
> + return PTR_ERR(acpd7219_clk);
> + }
> +
> + clk_disable_unprepare(acpd7219_clk);
> +
> return 0;
> }
> @@ -244,6 +292,112 @@ static int cz_fe_startup(struct snd_pcm_substream
*substream)
> .num_controls = ARRAY_SIZE(cz_mc_controls),
> };
> +#ifdef CONFIG_COMMON_CLK
> +static int acpd7219_clks_enable(struct clk_hw *hw)
> +{
> + u32 reg_val;
> + struct cz_clock *cz_clock_obj =
> + container_of(hw, struct cz_clock, acp_clks_hw);
What compile failure does the following cause?
struct cz_clock *cz_clock = container_of(hw, struct cz_clock,
acp_clks_hw);
> + void __iomem *base = cz_clock_obj->res_base;
> +
> + reg_val = readl(base + MISCCLKCNTL1);
> + reg_val &= ~(0x1 << OSCCLKENB);
> + writel(reg_val, base + MISCCLKCNTL1);
> + reg_val = readl(base + CLKDRVSTR2);
> + reg_val |= (0x1 << OSCOUT1CLK25MHZ);
> + writel(reg_val, base + CLKDRVSTR2);
> +
> + return 0;
> +}
> +
> +static void acpd7219_clks_disable(struct clk_hw *hw)
> +{
> + u32 reg_val;
> + struct cz_clock *cz_clock_obj =
> + container_of(hw, struct cz_clock, acp_clks_hw);
> + void __iomem *base = cz_clock_obj->res_base;
> +
> + reg_val = readl(base + MISCCLKCNTL1);
> + reg_val |= (0x1 << OSCCLKENB);
> + writel(reg_val, base + MISCCLKCNTL1);
> +}
> +
> +static int acpd7219_clks_is_enabled(struct clk_hw *hw)
> +{
> + u32 reg_val;
> + struct cz_clock *cz_clock_obj =
> + container_of(hw, struct cz_clock, acp_clks_hw);
> + void __iomem *base = cz_clock_obj->res_base;
> +
> + reg_val = readl(base + MISCCLKCNTL1);
> +
> + return !(reg_val & OSCCLKENB);
> +}
> +
> +static const struct clk_ops acpd7219_clks_ops = {
> + .enable = acpd7219_clks_enable,
> + .disable = acpd7219_clks_disable,
> + .is_enabled = acpd7219_clks_is_enabled,
> +};
> +
> +static int register_acpd7219_clocks(struct platform_device *pdev)
Where is the corresponding unregister? Isn't this a module that can be
unloaded?
> +{
> + struct clk *acp_clks;
> + struct clk_lookup *acp_clks_lookup;
> + struct cz_clock *cz_clock_obj;
> + struct resource *res;
> + struct device dev = pdev->dev;
struct device *dev = &pdev->dev;
Thanks,
-Dan
More information about the Alsa-devel
mailing list