| Message ID | 1522404297-7555-2-git-send-email-akshu.agrawal@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 30, 2018 at 03:34:57PM +0530, Akshu Agrawal 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. > > 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@amd.com> > Acked-by: Alex Deucher <alexander.deucher@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 > LGTM. Thanks, Sriram. > sound/soc/amd/acp-da7219-max98357a.c | 159 ++++++++++++++++++++++++++++++++++- > 1 file changed, 157 insertions(+), 2 deletions(-)
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. (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@amd.com> > Acked-by: Alex Deucher <alexander.deucher@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
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. For ST/CZ we have other platforms which do not use this clock. They have a dedicated Oscillator for the codec. 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. > > (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? > Accepted. This will make the code cleaner. > > 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@amd.com> >> Acked-by: Alex Deucher <alexander.deucher@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. > Accepted. will change it PLL_OUT. >> #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. > I don't think putting in struct is bad. Plus it helps me get res_base value on clk_enable using clk_hw. > Also why the 's' in "clks"? It looks like there is just one clock, so just > use the singular, acp_clk. > Accepted. >> +#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? > Agreed, there is ref counting for clk_get. Will do a single clk_get in register. >> + 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()? > Yes, all resources allocated by hw_paramns are freed in hw_free. >> 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); > Accepted. I misunderstood your earlier comment on this. >> + 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? > I see none of the machine driver have registered a remove callback, I wonder why? >> +{ >> + 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; > Accepted. > Thanks, > -Dan > Regards, Akshu
> 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. [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
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. > [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; >> +}; > Accepted. > [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. > """ > 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 3>[ 45.690976] BUG: sleeping function called from invalid context at /mnt/host/source/src/third_party/kernel/v4.14/kernel/locking/mutex.c:23 8 <3>[ 45.690988] in_atomic(): 1, irqs_disabled(): 1, pid: 2104, name: aplay <4>[ 45.690996] CPU: 1 PID: 2104 Comm: aplay Not tainted 4.14.31 #7 <4>[ 45.691001] Hardware name: Google Grunt/Grunt, BIOS Google_Grunt.10531.0.0 03/30/2018 <4>[ 45.691005] Call Trace: <4>[ 45.691022] dump_stack+0x4d/0x63 <4>[ 45.691033] ___might_sleep+0x11f/0x12e <4>[ 45.691040] mutex_lock+0x20/0x42 <4>[ 45.691049] regmap_update_bits_base+0x35/0x7c <4>[ 45.691056] snd_soc_component_update_bits+0x39/0x67 <4>[ 45.691064] ? __mutex_trylock_or_owner+0x4d/0x6a <4>[ 45.691073] da7219_dai_clks_prepare+0x27/0x2b [snd_soc_da7219] <4>[ 45.691081] clk_core_prepare+0xa8/0x122 <4>[ 45.691088] clk_prepare+0x21/0x2d <4>[ 45.691095] cz_da7219_trigger+0x45/0xfb [snd_soc_acp_da7219mx98357_mach] <4>[ 45.691101] soc_pcm_trigger+0xf6/0x112 > [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. > Accepted. > -Dan > Regards, Akshu
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. > >>> > >>> 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. -Dan
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
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 #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; +#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"); + 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); + 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); + 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) +{ + struct clk *acp_clks; + struct clk_lookup *acp_clks_lookup; + struct cz_clock *cz_clock_obj; + struct resource *res; + struct device dev = pdev->dev; + static const struct clk_init_data init = { + .name = "acpd7219-clks", + .ops = &acpd7219_clks_ops, + }; + + cz_clock_obj = kzalloc(sizeof(struct cz_clock), GFP_KERNEL); + if (!cz_clock_obj) + return -ENOMEM; + + cz_clock_obj->acp_clks_name = "acpd7219-clks"; + + cz_clock_obj->acp_clks_hw.init = &init; + + acp_clks = devm_clk_register(&dev, &cz_clock_obj->acp_clks_hw); + if (IS_ERR(acp_clks)) + { + dev_err(&dev, "Failed to register DAI clocks: %ld\n", + PTR_ERR(acp_clks)); + return PTR_ERR(acp_clks); + } + cz_clock_obj->acp_clks = acp_clks; + + acp_clks_lookup = clkdev_create(acp_clks, cz_clock_obj->acp_clks_name, + "%s", dev_name(&dev)); + if (!acp_clks_lookup) + dev_warn(&dev, "Failed to create DAI clkdev"); + else + cz_clock_obj->acp_clks_lookup = acp_clks_lookup; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) { + dev_err(&pdev->dev, "Failed to get misc io resource.\n"); + clkdev_drop(acp_clks_lookup); + return -EINVAL; + } + cz_clock_obj->res_base = devm_ioremap_nocache(&pdev->dev, res->start, + resource_size(res)); + if (!cz_clock_obj->res_base) { + clkdev_drop(acp_clks_lookup); + return -ENOMEM; + } + + return 0; +} +#else +static int register_acpd7219_clocks(struct platform_device *pdev) +{ + return 0; +} +#endif /* CONFIG_COMMON_CLK */ + static int cz_probe(struct platform_device *pdev) { int ret; @@ -259,7 +413,8 @@ static int cz_probe(struct platform_device *pdev) cz_card.name, ret); return ret; } - return 0; + + return register_acpd7219_clocks(pdev); } static const struct acpi_device_id cz_audio_acpi_match[] = {