ASoC: SOF: add DAI clock driver by brentlu · Pull Request #2932 · thesofproject/linux

brentlu · 2021-05-19T12:47:46Z

A virtual clock driver is added to SOF platform driver. Machine driver
could use the clock interface to send IPCs to SOF FW to enable/disable
the clocks.

Signed-off-by: Brent Lu brent.lu@intel.com

brentlu · 2021-05-19T12:58:51Z

+
+		init.name = pdata->dai_clks[i].name;
+		init.ops = &sof_clk_ops;
+		init.flags = CLK_GET_RATE_NOCACHE | CLK_SET_RATE_GATE;


The problem here is I don't know the correct flag for the virtual clock. Need expert to comment the flag.

not an expert in the clock API, but how about 0? :-) So far this clock doesn't even change rate, so why cannot it be cached?

RATE_GATE might be valid for SCLK they tend to like being stopped in order to be changed, but are we ever going to allow that?
if not then should we use fixed-clock for SOF clocks?

A virtual clock driver is added to SOF platform driver. Machine driver could use the clock interface to send IPCs to SOF FW to enable/disable the clocks. Signed-off-by: Brent Lu <brent.lu@intel.com>

lyakh · 2021-05-20T06:19:03Z

+	clkctrl.clk_en = enable ? 1 : 0;
+	clkctrl.clk_id = dai_clk->clk_id;
+
+	dev_dbg(sdev->dev, "clkctrl: SSP%d clk_en %d clk_id %d\n",


is this clock SSP-only? If it is, it should be clear at the file / function level, if it isn't, it shouldn't be hard-coded here

It might be better to be human readable:
dev_dbg(sdev->dev, "clkctrl: %s (clkid: %d) - %s\n", dai_clk->name, dai_clk->clk_id, enable ? "Enable" : "Disable");

lyakh · 2021-05-20T06:25:45Z

+
+		init.name = pdata->dai_clks[i].name;
+		init.ops = &sof_clk_ops;
+		init.flags = CLK_GET_RATE_NOCACHE | CLK_SET_RATE_GATE;


not an expert in the clock API, but how about 0? :-) So far this clock doesn't even change rate, so why cannot it be cached?

lyakh · 2021-05-20T06:26:59Z

+		}
+
+		dai_clk_lookup = clkdev_hw_create(&clkdev->hw, init.name,
+						  NULL);


NULL would comfortably fit onto the previous line

lyakh · 2021-05-20T06:33:28Z

+	do {
+		if (sdev->dai_clks_lookup[i])
+			clkdev_drop(sdev->dai_clks_lookup[i]);
+	} while (i-- > 0);


you can simplify this: from your above code whenever on iteration i you encounter an error and jump to err you know, that all sdev->dai_clks_lookup[0..i-1] have been created successfully, right? So, you could just

while (i-- > 0) clkdev_drop(sdev->dai_clks_lookup[i]);

or similar

lyakh · 2021-05-20T06:37:29Z

+	for (i = sdev->num_clks - 1; i >= 0; --i) {
+		if (sdev->dai_clks_lookup[i])
+			clkdev_drop(sdev->dai_clks_lookup[i]);
+	}


same here: I assume, this is only called if .probe() above was successful. In that case you know, that all sdev->dai_clks_lookup[0..sdev->num_clks-1] elements have been created successfully

lyakh · 2021-05-20T06:37:52Z

+	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
+	int i;
+
+	for (i = sdev->num_clks - 1; i >= 0; --i) {


just curious - any specific reason to run this in reverse order?

lyakh · 2021-05-20T06:42:07Z

+	uint32_t dai_index;	/**< index of this type dai */
+
+	uint32_t clk_en;	/**< zero to disable the clock */
+	uint32_t clk_id;	/**< clock id to operate */


how sure are we that this is all we'll ever need? Can it be that at some point we'll also need to send a rate down? Or maybe even the duty cycle (ok, maybe not this one)?

ujfalusi · 2021-05-24T07:12:59Z

I'm curious on how the clock API works on non DT world. How the machine driver would request the SCLK from one of the SSP ports the broken by design codec is connected to? Is it so that we just look for a unique clock name to request?
As a side note: it is not really the machine driver which should be handling the clock for the codec but the codec itself as it needs in order to be able to access registers. So whenever we want to write to the codec register we would need to clock (MCLK or/and SCLK) running.

dbaluta · 2021-05-24T07:38:07Z

+		struct sof_clk *clkdev;
+
+		clkdev = devm_kzalloc(dev, sizeof(*clkdev), GFP_KERNEL);
+		if (!clkdev) {


I think this operations should be platform specific. In IMX case we just have the clocks defined in the device tree file (dts) and then just get a reference to the clock here.

Also, with the IMX we enable and configure IP clocks (not only DAI clocks) on the Host side.

@dbaluta, you are right, but it should also depend on firmware version. Do we have the clock support enabled in the firmware?
At the end we will have SOF as a clock provider as well so platforms can move audio related clock implementations into the firmware and manage it via IPC.

@ujfalusi for IMX there is little to zero support for IPs clocks in the firmware. 2 out of 3 IMX supported platforms have a dedicated M4 core that is running a firmware which handles the clocks.
I like the idea of making the SOF (DSP) as a clock provider. It will make sense for IMX future platforms.

plbossart · 2021-05-24T12:59:30Z

@brentlu there is already an on-going work to start toggling the bclk during the hw_params stage and stop it during the hw_free stage. That would cover the current issues with most codecs and will be rather simple to backport.

I would like to push the introduction of new IPCs to a later stage when we have the SOF client support that @ujfalusi is working on.

plbossart · 2021-06-07T14:17:07Z

let's work on #2965 for now, we'll have to add clock control when we have the SOF client in place.

brentlu requested review from bardliao, dbaluta, juimonen, keyonjie, kv2019i, lgirdwood, libinyang, lyakh, plbossart, ranj063 and singalsu as code owners May 19, 2021 12:47

brentlu linked an issue May 19, 2021 that may be closed by this pull request

[FEATURE] Explicit clock control #2907

Closed

brentlu mentioned this pull request May 19, 2021

[FEATURE] Explicit clock control thesofproject/sof#4156

Closed

brentlu commented May 19, 2021

View reviewed changes

brentlu mentioned this pull request May 20, 2021

dai: support explicit clock control thesofproject/sof#4213

Closed

ASoC: SOF: add DAI clock driver

9e996a3

A virtual clock driver is added to SOF platform driver. Machine driver could use the clock interface to send IPCs to SOF FW to enable/disable the clocks. Signed-off-by: Brent Lu <brent.lu@intel.com>

brentlu force-pushed the dai_clk_driver branch from 67ac7dd to 9e996a3 Compare May 20, 2021 01:48

lyakh requested changes May 20, 2021

View reviewed changes

brentlu marked this pull request as draft May 20, 2021 09:28

kv2019i requested a review from ujfalusi May 20, 2021 14:46

dbaluta reviewed May 24, 2021

View reviewed changes

plbossart closed this Jun 7, 2021

Conversation

brentlu commented May 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi commented May 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbaluta May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart commented May 24, 2021

Uh oh!

plbossart commented Jun 7, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dbaluta May 24, 2021 •

edited

Loading