diff mbox series

[3/5] ASoC: xilinx: xlnx_i2s.c: Handle sysclk setting

Message ID 20220105225146.3517039-4-robert.hancock@calian.com (mailing list archive)
State Superseded
Headers show
Series ASoC: Xilinx fixes | expand

Commit Message

Robert Hancock Jan. 5, 2022, 10:51 p.m. UTC 
This driver previously only handled the set_clkdiv divider callback when
setting the SCLK Out Divider field in the I2S Timing Control register.
However, when using the simple-audio-card driver, the set_sysclk function
is called but not set_clkdiv. This caused the divider not to be set,
leaving it at an invalid value of 0 and resulting in a very low SCLK
output rate.

Handle set_clkdiv and store the sysclk (MCLK) value for later use in
hw_params to set the SCLK Out Divider such that:
MCLK/SCLK = divider * 2

Fixes: 112a8900d4b0 ("ASoC: xlnx: Add i2s driver")
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 sound/soc/xilinx/xlnx_i2s.c | 104 ++++++++++++++++++++++++------------
 1 file changed, 70 insertions(+), 34 deletions(-)

Comments

Mark Brown Jan. 6, 2022, 12:42 p.m. UTC | #1 
On Wed, Jan 05, 2022 at 04:51:44PM -0600, Robert Hancock wrote:

> +	unsigned int last_sysclk;

Same naming issue.

> +	if (drv_data->last_sysclk) {
> +		unsigned int bits_per_sample = drv_data->is_32bit_lrclk ?
> +					       32 : drv_data->data_width;

Please write normal conditional statements, it improves legibility.

> +		unsigned int sclk = params_rate(params) * bits_per_sample *
> +				    params_channels(params);

snd_soc_params_to_bclk().

> +		unsigned int sclk_div = DIV_ROUND_CLOSEST(drv_data->last_sysclk, sclk) / 2;

Same issue with _ROUND_CLOSEST()

> +
> +		if (!sclk_div || (sclk_div & ~I2S_I2STIM_VALID_MASK)) {
> +			dev_warn(i2s_dai->dev, "invalid SCLK divisor for sysclk %u and sclk %u\n",
> +				 drv_data->last_sysclk, sclk);
> +			return -EINVAL;
> +		}

This indicates that we should be setting some constraints on sample rate
based on sysclk.

> +		writel(sclk_div, drv_data->base + I2S_I2STIM_OFFSET);
> +	}

Does the device actually support operation without knowing the sysclk?

> @@ -56,18 +90,18 @@ static int xlnx_i2s_hw_params(struct snd_pcm_substream *substream,
>  static int xlnx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
>  			    struct snd_soc_dai *i2s_dai)
>  {
> -	void __iomem *base = snd_soc_dai_get_drvdata(i2s_dai);
> +	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(i2s_dai);
>  
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> -		writel(1, base + I2S_CORE_CTRL_OFFSET);
> +		writel(I2S_CORE_CTRL_ENABLE, drv_data->base + I2S_CORE_CTRL_OFFSET);
>  		break;
>  	case SNDRV_PCM_TRIGGER_STOP:
>  	case SNDRV_PCM_TRIGGER_SUSPEND:
>  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> -		writel(0, base + I2S_CORE_CTRL_OFFSET);
> +		writel(0, drv_data->base + I2S_CORE_CTRL_OFFSET);
>  		break;

Please split the change to have a struct for driver data out into a
separate change, it's a large reformatting and is big enough to justify
it - more of the diff is that than the change described in the changelog
which doesn't mention this at all.
Robert Hancock Jan. 6, 2022, 11:25 p.m. UTC | #2 
On Thu, 2022-01-06 at 12:42 +0000, Mark Brown wrote:
> On Wed, Jan 05, 2022 at 04:51:44PM -0600, Robert Hancock wrote:
> 
> > +	unsigned int last_sysclk;
> 
> Same naming issue.

Will switch to sysclk here as well.

> 
> > +	if (drv_data->last_sysclk) {
> > +		unsigned int bits_per_sample = drv_data->is_32bit_lrclk ?
> > +					       32 : drv_data->data_width;
> 
> Please write normal conditional statements, it improves legibility.

Will do.

> 
> > +		unsigned int sclk = params_rate(params) * bits_per_sample *
> > +				    params_channels(params);
> 
> snd_soc_params_to_bclk().

I don't think that works here since that depends on the result of
snd_soc_params_to_frame_size, which doesn't account for the bits per sample
being forced to 32 when the 32bit_lrclk mode is active?

> 
> > +		unsigned int sclk_div = DIV_ROUND_CLOSEST(drv_data-
> > >last_sysclk, sclk) / 2;
> 
> Same issue with _ROUND_CLOSEST()

Will update to require exact divisibility.

> 
> > +
> > +		if (!sclk_div || (sclk_div & ~I2S_I2STIM_VALID_MASK)) {
> > +			dev_warn(i2s_dai->dev, "invalid SCLK divisor for sysclk
> > %u and sclk %u\n",
> > +				 drv_data->last_sysclk, sclk);
> > +			return -EINVAL;
> > +		}
> 
> This indicates that we should be setting some constraints on sample rate
> based on sysclk.

Is there a way to do this at this level given that it can only be enforced
after set_sysclk is called? Most of the other drivers that enforce rate
constraints seem to be more of a fixed list..

> 
> > +		writel(sclk_div, drv_data->base + I2S_I2STIM_OFFSET);
> > +	}
> 
> Does the device actually support operation without knowing the sysclk?

It could work if set_clkdiv is called to directly set the divider, rather than
set_sysclk. simple-card doesn't do that, but possibly some other setup which
uses this does?

> 
> > @@ -56,18 +90,18 @@ static int xlnx_i2s_hw_params(struct snd_pcm_substream
> > *substream,
> >  static int xlnx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> >  			    struct snd_soc_dai *i2s_dai)
> >  {
> > -	void __iomem *base = snd_soc_dai_get_drvdata(i2s_dai);
> > +	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(i2s_dai);
> >  
> >  	switch (cmd) {
> >  	case SNDRV_PCM_TRIGGER_START:
> >  	case SNDRV_PCM_TRIGGER_RESUME:
> >  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> > -		writel(1, base + I2S_CORE_CTRL_OFFSET);
> > +		writel(I2S_CORE_CTRL_ENABLE, drv_data->base +
> > I2S_CORE_CTRL_OFFSET);
> >  		break;
> >  	case SNDRV_PCM_TRIGGER_STOP:
> >  	case SNDRV_PCM_TRIGGER_SUSPEND:
> >  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> > -		writel(0, base + I2S_CORE_CTRL_OFFSET);
> > +		writel(0, drv_data->base + I2S_CORE_CTRL_OFFSET);
> >  		break;
> 
> Please split the change to have a struct for driver data out into a
> separate change, it's a large reformatting and is big enough to justify
> it - more of the diff is that than the change described in the changelog
> which doesn't mention this at all.

Will do.
Mark Brown Jan. 7, 2022, 1:31 p.m. UTC | #3 
On Thu, Jan 06, 2022 at 11:25:28PM +0000, Robert Hancock wrote:
> On Thu, 2022-01-06 at 12:42 +0000, Mark Brown wrote:
> > On Wed, Jan 05, 2022 at 04:51:44PM -0600, Robert Hancock wrote:

> > snd_soc_params_to_bclk().

> I don't think that works here since that depends on the result of
> snd_soc_params_to_frame_size, which doesn't account for the bits per sample
> being forced to 32 when the 32bit_lrclk mode is active?

OK.

> > > +		if (!sclk_div || (sclk_div & ~I2S_I2STIM_VALID_MASK)) {
> > > +			dev_warn(i2s_dai->dev, "invalid SCLK divisor for sysclk
> > > %u and sclk %u\n",
> > > +				 drv_data->last_sysclk, sclk);
> > > +			return -EINVAL;
> > > +		}

> > This indicates that we should be setting some constraints on sample rate
> > based on sysclk.

> Is there a way to do this at this level given that it can only be enforced
> after set_sysclk is called? Most of the other drivers that enforce rate
> constraints seem to be more of a fixed list..

	if (drvdata->sysclk) {
		/* set constraints */
	}


like a bunch of other drivers do (eg, wm8731).

> > > +		writel(sclk_div, drv_data->base + I2S_I2STIM_OFFSET);
> > > +	}

> > Does the device actually support operation without knowing the sysclk?

> It could work if set_clkdiv is called to directly set the divider, rather than
> set_sysclk. simple-card doesn't do that, but possibly some other setup which
> uses this does?

We should be migrating away from set_clkdiv() anyway, it'll be fine to
require that such drivers be updated.
diff mbox series

Patch

diff --git a/sound/soc/xilinx/xlnx_i2s.c b/sound/soc/xilinx/xlnx_i2s.c
index cc641e582c82..1abe28821916 100644
--- a/sound/soc/xilinx/xlnx_i2s.c
+++ b/sound/soc/xilinx/xlnx_i2s.c
@@ -18,20 +18,39 @@ 
 #define DRV_NAME "xlnx_i2s"
 
 #define I2S_CORE_CTRL_OFFSET		0x08
+#define I2S_CORE_CTRL_32BIT_LRCLK	BIT(3)
+#define I2S_CORE_CTRL_ENABLE		BIT(0)
 #define I2S_I2STIM_OFFSET		0x20
 #define I2S_CH0_OFFSET			0x30
 #define I2S_I2STIM_VALID_MASK		GENMASK(7, 0)
 
+struct xlnx_i2s_drv_data {
+	struct snd_soc_dai_driver dai_drv;
+	void __iomem *base;
+	unsigned int last_sysclk;
+	u32 data_width;
+	bool is_32bit_lrclk;
+};
+
 static int xlnx_i2s_set_sclkout_div(struct snd_soc_dai *cpu_dai,
 				    int div_id, int div)
 {
-	void __iomem *base = snd_soc_dai_get_drvdata(cpu_dai);
+	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(cpu_dai);
 
 	if (!div || (div & ~I2S_I2STIM_VALID_MASK))
 		return -EINVAL;
 
-	writel(div, base + I2S_I2STIM_OFFSET);
+	writel(div, drv_data->base + I2S_I2STIM_OFFSET);
+
+	return 0;
+}
+
+static int xlnx_i2s_set_sysclk(struct snd_soc_dai *dai,
+			       int clk_id, unsigned int freq, int dir)
+{
+	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(dai);
 
+	drv_data->last_sysclk = freq;
 	return 0;
 }
 
@@ -40,13 +59,28 @@  static int xlnx_i2s_hw_params(struct snd_pcm_substream *substream,
 			      struct snd_soc_dai *i2s_dai)
 {
 	u32 reg_off, chan_id;
-	void __iomem *base = snd_soc_dai_get_drvdata(i2s_dai);
+	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(i2s_dai);
+
+	if (drv_data->last_sysclk) {
+		unsigned int bits_per_sample = drv_data->is_32bit_lrclk ?
+					       32 : drv_data->data_width;
+		unsigned int sclk = params_rate(params) * bits_per_sample *
+				    params_channels(params);
+		unsigned int sclk_div = DIV_ROUND_CLOSEST(drv_data->last_sysclk, sclk) / 2;
+
+		if (!sclk_div || (sclk_div & ~I2S_I2STIM_VALID_MASK)) {
+			dev_warn(i2s_dai->dev, "invalid SCLK divisor for sysclk %u and sclk %u\n",
+				 drv_data->last_sysclk, sclk);
+			return -EINVAL;
+		}
+		writel(sclk_div, drv_data->base + I2S_I2STIM_OFFSET);
+	}
 
 	chan_id = params_channels(params) / 2;
 
 	while (chan_id > 0) {
 		reg_off = I2S_CH0_OFFSET + ((chan_id - 1) * 4);
-		writel(chan_id, base + reg_off);
+		writel(chan_id, drv_data->base + reg_off);
 		chan_id--;
 	}
 
@@ -56,18 +90,18 @@  static int xlnx_i2s_hw_params(struct snd_pcm_substream *substream,
 static int xlnx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 			    struct snd_soc_dai *i2s_dai)
 {
-	void __iomem *base = snd_soc_dai_get_drvdata(i2s_dai);
+	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(i2s_dai);
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		writel(1, base + I2S_CORE_CTRL_OFFSET);
+		writel(I2S_CORE_CTRL_ENABLE, drv_data->base + I2S_CORE_CTRL_OFFSET);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		writel(0, base + I2S_CORE_CTRL_OFFSET);
+		writel(0, drv_data->base + I2S_CORE_CTRL_OFFSET);
 		break;
 	default:
 		return -EINVAL;
@@ -78,6 +112,7 @@  static int xlnx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 
 static const struct snd_soc_dai_ops xlnx_i2s_dai_ops = {
 	.trigger = xlnx_i2s_trigger,
+	.set_sysclk = xlnx_i2s_set_sysclk,
 	.set_clkdiv = xlnx_i2s_set_sclkout_div,
 	.hw_params = xlnx_i2s_hw_params
 };
@@ -95,20 +130,19 @@  MODULE_DEVICE_TABLE(of, xlnx_i2s_of_match);
 
 static int xlnx_i2s_probe(struct platform_device *pdev)
 {
-	void __iomem *base;
-	struct snd_soc_dai_driver *dai_drv;
+	struct xlnx_i2s_drv_data *drv_data;
 	int ret;
-	u32 ch, format, data_width;
+	u32 ch, format;
 	struct device *dev = &pdev->dev;
 	struct device_node *node = dev->of_node;
 
-	dai_drv = devm_kzalloc(&pdev->dev, sizeof(*dai_drv), GFP_KERNEL);
-	if (!dai_drv)
+	drv_data = devm_kzalloc(&pdev->dev, sizeof(*drv_data), GFP_KERNEL);
+	if (!drv_data)
 		return -ENOMEM;
 
-	base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
+	drv_data->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(drv_data->base))
+		return PTR_ERR(drv_data->base);
 
 	ret = of_property_read_u32(node, "xlnx,num-channels", &ch);
 	if (ret < 0) {
@@ -117,12 +151,12 @@  static int xlnx_i2s_probe(struct platform_device *pdev)
 	}
 	ch = ch * 2;
 
-	ret = of_property_read_u32(node, "xlnx,dwidth", &data_width);
+	ret = of_property_read_u32(node, "xlnx,dwidth", &drv_data->data_width);
 	if (ret < 0) {
 		dev_err(dev, "cannot get data width\n");
 		return ret;
 	}
-	switch (data_width) {
+	switch (drv_data->data_width) {
 	case 16:
 		format = SNDRV_PCM_FMTBIT_S16_LE;
 		break;
@@ -134,35 +168,37 @@  static int xlnx_i2s_probe(struct platform_device *pdev)
 	}
 
 	if (of_device_is_compatible(node, "xlnx,i2s-transmitter-1.0")) {
-		dai_drv->name = "xlnx_i2s_playback";
-		dai_drv->playback.stream_name = "Playback";
-		dai_drv->playback.formats = format;
-		dai_drv->playback.channels_min = ch;
-		dai_drv->playback.channels_max = ch;
-		dai_drv->playback.rates	= SNDRV_PCM_RATE_8000_192000;
-		dai_drv->ops = &xlnx_i2s_dai_ops;
+		drv_data->dai_drv.name = "xlnx_i2s_playback";
+		drv_data->dai_drv.playback.stream_name = "Playback";
+		drv_data->dai_drv.playback.formats = format;
+		drv_data->dai_drv.playback.channels_min = ch;
+		drv_data->dai_drv.playback.channels_max = ch;
+		drv_data->dai_drv.playback.rates	= SNDRV_PCM_RATE_8000_192000;
+		drv_data->dai_drv.ops = &xlnx_i2s_dai_ops;
 	} else if (of_device_is_compatible(node, "xlnx,i2s-receiver-1.0")) {
-		dai_drv->name = "xlnx_i2s_capture";
-		dai_drv->capture.stream_name = "Capture";
-		dai_drv->capture.formats = format;
-		dai_drv->capture.channels_min = ch;
-		dai_drv->capture.channels_max = ch;
-		dai_drv->capture.rates = SNDRV_PCM_RATE_8000_192000;
-		dai_drv->ops = &xlnx_i2s_dai_ops;
+		drv_data->dai_drv.name = "xlnx_i2s_capture";
+		drv_data->dai_drv.capture.stream_name = "Capture";
+		drv_data->dai_drv.capture.formats = format;
+		drv_data->dai_drv.capture.channels_min = ch;
+		drv_data->dai_drv.capture.channels_max = ch;
+		drv_data->dai_drv.capture.rates = SNDRV_PCM_RATE_8000_192000;
+		drv_data->dai_drv.ops = &xlnx_i2s_dai_ops;
 	} else {
 		return -ENODEV;
 	}
+	drv_data->is_32bit_lrclk = readl(drv_data->base + I2S_CORE_CTRL_OFFSET) &
+				   I2S_CORE_CTRL_32BIT_LRCLK;
 
-	dev_set_drvdata(&pdev->dev, base);
+	dev_set_drvdata(&pdev->dev, drv_data);
 
 	ret = devm_snd_soc_register_component(&pdev->dev, &xlnx_i2s_component,
-					      dai_drv, 1);
+					      &drv_data->dai_drv, 1);
 	if (ret) {
 		dev_err(&pdev->dev, "i2s component registration failed\n");
 		return ret;
 	}
 
-	dev_info(&pdev->dev, "%s DAI registered\n", dai_drv->name);
+	dev_info(&pdev->dev, "%s DAI registered\n", drv_data->dai_drv.name);
 
 	return ret;
 }