2021-09-02 13:42:31

by Péter Ujfalusi

[permalink] [raw]
Subject: Re: [PATCH 1/2] ASoC: SOF: imx: Add code to manage DSP related clocks

Hi Daniel,

On 02/09/2021 15:32, Daniel Baluta wrote:
> From: Daniel Baluta <[email protected]>
>
> There are two types of clocks:
> * DSP IP clocks
> * DAI clocks
>
> This clocks are necessary in order to power up DSP and DAIs.
>
> We choose to enable DAI clocks here because of the way i.MX8/i.MX8X
> design handles resources (including clocks).
>
> All clocks are managed by a separate core (named SCU) which communicates
> with Linux managed ARM core via a well known API.
>
> We parse and enable the clocks in probe function and disable them in
> remove function.
>
> Future patches will introduce Power Management support so that we
> disable clocks while DSP is not used or system enters power save.

Unfortunately this patch does not apply to next.

I might be a bit too cautius, but I would also add "&& COMMON_CLK" for
the COMPILE_TEST in Kconfig or select it from where it is appropriate?

> Signed-off-by: Daniel Baluta <[email protected]>
> ---
> sound/soc/sof/imx/imx-common.c | 77 ++++++++++++++++++++++++++++++++++
> sound/soc/sof/imx/imx-common.h | 16 +++++++
> sound/soc/sof/imx/imx8.c | 32 ++++++++++++++
> sound/soc/sof/imx/imx8m.c | 33 +++++++++++++++
> 4 files changed, 158 insertions(+)
>
> diff --git a/sound/soc/sof/imx/imx8m.c b/sound/soc/sof/imx/imx8m.c
> index 30624fafc632..482c25ab15ce 100644
> --- a/sound/soc/sof/imx/imx8m.c
> +++ b/sound/soc/sof/imx/imx8m.c
> @@ -23,6 +23,20 @@
> #define MBOX_OFFSET 0x800000
> #define MBOX_SIZE 0x1000
>
> +#define IMX8M_DSP_CLK_NUM 3
> +static const char *imx8m_dsp_clks_names[IMX8M_DSP_CLK_NUM] = {

static const char *imx8m_dsp_clks_names[]

+ ARRAY_SIZE(imx8m_dsp_clks_names) instead IMX8M_DSP_CLK_NUM ?


--
Péter


2021-09-03 16:18:03

by Daniel Baluta

[permalink] [raw]
Subject: Re: [PATCH 1/2] ASoC: SOF: imx: Add code to manage DSP related clocks

On Thu, Sep 2, 2021 at 11:02 PM Péter Ujfalusi
<[email protected]> wrote:
>
> Hi Daniel,
>
> On 02/09/2021 15:32, Daniel Baluta wrote:
> > From: Daniel Baluta <[email protected]>
> >
> > There are two types of clocks:
> > * DSP IP clocks
> > * DAI clocks
> >
> > This clocks are necessary in order to power up DSP and DAIs.
> >
> > We choose to enable DAI clocks here because of the way i.MX8/i.MX8X
> > design handles resources (including clocks).
> >
> > All clocks are managed by a separate core (named SCU) which communicates
> > with Linux managed ARM core via a well known API.
> >
> > We parse and enable the clocks in probe function and disable them in
> > remove function.
> >
> > Future patches will introduce Power Management support so that we
> > disable clocks while DSP is not used or system enters power save.
>
> Unfortunately this patch does not apply to next.

Yes, because my patch is based on SOF topic/sof-dev branch and this small patch

https://github.com/thesofproject/linux/commit/b56c58b5938a626fb08fcf1d5e38d687b520ab89

is not in linux-next.

I plan to stay on SOF branch and get the review tags so we can merge
it in SOF tree.
>
> I might be a bit too cautius, but I would also add "&& COMMON_CLK" for
> the COMPILE_TEST in Kconfig or select it from where it is appropriate?

Maybe add a depends on COMMON_CLK for IMX hardware support? Altough,
if CLK support
is not selected clk API transforms itself into dummy wrappers.
>
> > Signed-off-by: Daniel Baluta <[email protected]>
> > ---
> > sound/soc/sof/imx/imx-common.c | 77 ++++++++++++++++++++++++++++++++++
> > sound/soc/sof/imx/imx-common.h | 16 +++++++
> > sound/soc/sof/imx/imx8.c | 32 ++++++++++++++
> > sound/soc/sof/imx/imx8m.c | 33 +++++++++++++++
> > 4 files changed, 158 insertions(+)
> >
> > diff --git a/sound/soc/sof/imx/imx8m.c b/sound/soc/sof/imx/imx8m.c
> > index 30624fafc632..482c25ab15ce 100644
> > --- a/sound/soc/sof/imx/imx8m.c
> > +++ b/sound/soc/sof/imx/imx8m.c
> > @@ -23,6 +23,20 @@
> > #define MBOX_OFFSET 0x800000
> > #define MBOX_SIZE 0x1000
> >
> > +#define IMX8M_DSP_CLK_NUM 3
> > +static const char *imx8m_dsp_clks_names[IMX8M_DSP_CLK_NUM] = {
>
> static const char *imx8m_dsp_clks_names[]
>
> + ARRAY_SIZE(imx8m_dsp_clks_names) instead IMX8M_DSP_CLK_NUM ?

Yes, this is a good idea. Already fixed in v2 I sent eariler.