Re: [PATCH v3 4/4] clk: fsl-sai: Add MCLK generation support

[Brian Masney <bmasney@redhat.com>]

Hi Marek,

On Sat, Apr 04, 2026 at 08:33:28PM +0200, Marek Vasut wrote:
> The driver currently supports generating BCLK. There are systems which
> require generation of MCLK instead. Register new MCLK clock and handle
> clock-cells = <1> to differentiate between BCLK and MCLK. In case of a
> legacy system with clock-cells = <0>, the driver behaves as before, i.e.
> always returns BCLK.
> 
> Note that it is not possible re-use the current SAI audio driver to
> generate MCLK and correctly enable and disable the MCLK.
> 
> If SAI (audio driver) is used to control the MCLK enablement, then MCLK
> clock is not always enabled, and it is not necessarily enabled when the
> codec may need the clock to be enabled. There is also no way for the
> codec node to specify phandle to clock provider in DT, because the SAI
> (audio driver) is not clock provider.
> 
> If SAI (clock driver) is used to control the MCLK enablement, then MCLK
> clock is enabled when the codec needs the clock enabled, because the
> codec is the clock consumer and the SAI (clock driver) is the clock
> provider, and the codec driver can request the clock to be enabled when
> needed. There is also the usual phandle to clock provider in DT, because
> the SAI (clock driver) is clock provider.
> 
> Acked-by: Michael Walle <mwalle@kernel.org>
> Signed-off-by: Marek Vasut <marex@nabladev.com>
> ---
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Michael Walle <michael@walle.cc>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-clk@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> V2: No change
> V3: - Rebase on current next, update mail address
>     - Update commit message according to clarify the difference between
>       SAI audio and SAI clock driver
>     - Pick ancient AB from Michael, although this may be outdated
>       https://patchwork.kernel.org/project/alsa-devel/patch/20241226162234.40141-4-marex@denx.de/
> ---
>  drivers/clk/clk-fsl-sai.c | 74 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 61 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/clk/clk-fsl-sai.c b/drivers/clk/clk-fsl-sai.c
> index 336aa8477d0ea..f00b49edb2e9f 100644
> --- a/drivers/clk/clk-fsl-sai.c
> +++ b/drivers/clk/clk-fsl-sai.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/err.h>
>  #include <linux/of.h>

Since changes are being made here, could the includes be sorted in a
separate patch?

> @@ -15,21 +16,37 @@
>  
>  #define I2S_CSR		0x00
>  #define I2S_CR2		0x08
> +#define I2S_MCR		0x100
>  #define CSR_BCE_BIT	28
> +#define CSR_TE_BIT	31
>  #define CR2_BCD		BIT(24)
>  #define CR2_DIV_SHIFT	0
>  #define CR2_DIV_WIDTH	8
> +#define MCR_MOE		BIT(30)
>  
>  struct fsl_sai_clk {
> -	struct clk_divider div;
> -	struct clk_gate gate;
> +	struct clk_divider bclk_div;
> +	struct clk_divider mclk_div;
> +	struct clk_gate bclk_gate;
> +	struct clk_gate mclk_gate;
> +	struct clk_hw *bclk_hw;
> +	struct clk_hw *mclk_hw;
>  	spinlock_t lock;
>  };
>  
>  struct fsl_sai_data {
>  	unsigned int	offset;	/* Register offset */
> +	bool		have_mclk; /* Have MCLK control */
>  };
>  
> +static struct clk_hw *
> +fsl_sai_of_clk_get(struct of_phandle_args *clkspec, void *data)
> +{
> +	struct fsl_sai_clk *sai_clk = data;
> +
> +	return clkspec->args[0] ? sai_clk->mclk_hw : sai_clk->bclk_hw;
> +}
> +
>  static int fsl_sai_clk_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -54,37 +71,68 @@ static int fsl_sai_clk_probe(struct platform_device *pdev)
>  
>  	spin_lock_init(&sai_clk->lock);
>  
> -	sai_clk->gate.reg = base + data->offset + I2S_CSR;
> -	sai_clk->gate.bit_idx = CSR_BCE_BIT;
> -	sai_clk->gate.lock = &sai_clk->lock;
> +	sai_clk->bclk_gate.reg = base + data->offset + I2S_CSR;
> +	sai_clk->bclk_gate.bit_idx = CSR_BCE_BIT;
> +	sai_clk->bclk_gate.lock = &sai_clk->lock;
>  
> -	sai_clk->div.reg = base + data->offset + I2S_CR2;
> -	sai_clk->div.shift = CR2_DIV_SHIFT;
> -	sai_clk->div.width = CR2_DIV_WIDTH;
> -	sai_clk->div.lock = &sai_clk->lock;
> +	sai_clk->bclk_div.reg = base + data->offset + I2S_CR2;
> +	sai_clk->bclk_div.shift = CR2_DIV_SHIFT;
> +	sai_clk->bclk_div.width = CR2_DIV_WIDTH;
> +	sai_clk->bclk_div.lock = &sai_clk->lock;
>  
>  	/* set clock direction, we are the BCLK master */
>  	writel(CR2_BCD, base + data->offset + I2S_CR2);
>  
> -	hw = devm_clk_hw_register_composite_pdata(dev, dev->of_node->name,
> +	hw = devm_clk_hw_register_composite_pdata(dev, "BCLK",

Will there ever be multiple instances of this IP block on a system? If
so, the hardcoded BCLK and MCLK clk names will collide.

>  						  &pdata, 1, NULL, NULL,
> -						  &sai_clk->div.hw,
> +						  &sai_clk->bclk_div.hw,
>  						  &clk_divider_ops,
> -						  &sai_clk->gate.hw,
> +						  &sai_clk->bclk_gate.hw,
>  						  &clk_gate_ops,
>  						  CLK_SET_RATE_GATE);
>  	if (IS_ERR(hw))
>  		return PTR_ERR(hw);
>  
> -	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
> +	sai_clk->bclk_hw = hw;
> +
> +	if (data->have_mclk) {
> +		sai_clk->mclk_gate.reg = base + data->offset + I2S_CSR;
> +		sai_clk->mclk_gate.bit_idx = CSR_TE_BIT;
> +		sai_clk->mclk_gate.lock = &sai_clk->lock;
> +
> +		sai_clk->mclk_div.reg = base + I2S_MCR;
> +		sai_clk->mclk_div.shift = CR2_DIV_SHIFT;
> +		sai_clk->mclk_div.width = CR2_DIV_WIDTH;
> +		sai_clk->mclk_div.lock = &sai_clk->lock;
> +
> +		pdata.index = 1; /* MCLK1 */
> +		hw = devm_clk_hw_register_composite_pdata(dev, "MCLK",
> +							  &pdata, 1, NULL, NULL,
> +							  &sai_clk->mclk_div.hw,
> +							  &clk_divider_ops,
> +							  &sai_clk->mclk_gate.hw,
> +							  &clk_gate_ops,
> +							  CLK_SET_RATE_GATE);
> +		if (IS_ERR(hw))
> +			return PTR_ERR(hw);
> +
> +		sai_clk->mclk_hw = hw;

This section is basically a copy/paste of the bclk from above, with a
few minor changes (bclk renamed to mclk, different gate and div regs,
and a different base offset). What do you think about mostly keeping
struct fsl_sai_clk with the same names, and add a new helper to do
the registrations?

Brian


> +
> +		/* set clock direction, we are the MCLK output */
> +		writel(MCR_MOE, base + I2S_MCR);
> +	}
> +
> +	return devm_of_clk_add_hw_provider(dev, fsl_sai_of_clk_get, sai_clk);
>  }
>  
>  static const struct fsl_sai_data fsl_sai_vf610_data = {
>  	.offset	= 0,
> +	.have_mclk = false,
>  };
>  
>  static const struct fsl_sai_data fsl_sai_imx8mq_data = {
>  	.offset	= 8,
> +	.have_mclk = true,
>  };
>  
>  static const struct of_device_id of_fsl_sai_clk_ids[] = {
> -- 
> 2.53.0
>