| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 22 Jun 2022 02:02:41 +0800
From: Chen-Yu Tsai <wens@...nel.org>
To: Judy Hsiao <judyhsiao@...omium.org>
Cc: Heiko Stuebner <heiko@...ech.de>,
Liam Girdwood <lgirdwood@...il.com>,
Rob Herring <robh+dt@...nel.org>,
Brian Norris <briannorris@...omium.org>,
Mark Brown <broonie@...nel.org>,
Jaroslav Kysela <perex@...ex.cz>,
Chen-Yu Tsai <wenst@...omium.org>,
Linux-ALSA <alsa-devel@...a-project.org>,
linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
"open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 1/3] ASoC: rockchip: i2s: switch BCLK to GPIO
Hi,
On Sun, Jun 19, 2022 at 5:54 PM Judy Hsiao <judyhsiao@...omium.org> wrote:
>
> We discoverd that the state of BCLK on, LRCLK off and SD_MODE on
> may cause the speaker melting issue. Removing LRCLK while BCLK
> is present can cause unexpected output behavior including a large
> DC output voltage as described in the Max98357a datasheet.
>
> In order to:
> 1. prevent BCLK from turning on by other component.
> 2. keep BCLK and LRCLK being present at the same time
>
> This patch switches BCLK to GPIO func before LRCLK output, and
> configures BCLK func back during LRCLK is output.
>
> Without this fix, BCLK is turned on 11 ms earlier than LRCK by the
> da7219.
> With this fix, BCLK is turned on only 0.4 ms earlier than LRCK by
> the rockchip codec.
>
> Signed-off-by: Judy Hsiao <judyhsiao@...omium.org>
> Reviewed-by: Brian Norris <briannorris@...omium.org>
> ---
> sound/soc/rockchip/rockchip_i2s.c | 171 ++++++++++++++++++++++--------
> 1 file changed, 124 insertions(+), 47 deletions(-)
>
> diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
> index 47a3971a9ce1..8e4a9b8746fd 100644
> --- a/sound/soc/rockchip/rockchip_i2s.c
> +++ b/sound/soc/rockchip/rockchip_i2s.c
> @@ -54,8 +54,38 @@ struct rk_i2s_dev {
> const struct rk_i2s_pins *pins;
> unsigned int bclk_ratio;
> spinlock_t lock; /* tx/rx lock */
> + struct pinctrl *pinctrl;
> + struct pinctrl_state *bclk_on;
> + struct pinctrl_state *bclk_off;
> };
>
> +static int i2s_pinctrl_select_bclk_on(struct rk_i2s_dev *i2s)
> +{
> + int ret = 0;
> +
> + if (!IS_ERR(i2s->pinctrl) && !IS_ERR_OR_NULL(i2s->bclk_on))
> + ret = pinctrl_select_state(i2s->pinctrl, i2s->bclk_on);
> +
> + if (ret)
> + dev_err(i2s->dev, "bclk enable failed %d\n", ret);
> +
> + return ret;
> +}
> +
> +static int i2s_pinctrl_select_bclk_off(struct rk_i2s_dev *i2s)
> +{
> +
> + int ret = 0;
> +
> + if (!IS_ERR(i2s->pinctrl) && !IS_ERR_OR_NULL(i2s->bclk_off))
> + ret = pinctrl_select_state(i2s->pinctrl, i2s->bclk_off);
> +
> + if (ret)
> + dev_err(i2s->dev, "bclk disable failed %d\n", ret);
> +
> + return ret;
> +}
> +
> static int i2s_runtime_suspend(struct device *dev)
> {
> struct rk_i2s_dev *i2s = dev_get_drvdata(dev);
> @@ -92,39 +122,46 @@ static inline struct rk_i2s_dev *to_info(struct snd_soc_dai *dai)
> return snd_soc_dai_get_drvdata(dai);
> }
>
> -static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
> +static int rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
> {
> unsigned int val = 0;
> int retry = 10;
> -
> + int ret = 0;
> +
> spin_lock(&i2s->lock);
> if (on) {
> - regmap_update_bits(i2s->regmap, I2S_DMACR,
> - I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_ENABLE);
> -
> - regmap_update_bits(i2s->regmap, I2S_XFER,
> - I2S_XFER_TXS_START | I2S_XFER_RXS_START,
> - I2S_XFER_TXS_START | I2S_XFER_RXS_START);
> -
> + ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
> + I2S_DMACR_TDE_ENABLE,
> + I2S_DMACR_TDE_ENABLE);
> + if (ret < 0)
> + goto end;
> + ret = regmap_update_bits(i2s->regmap, I2S_XFER,
> + I2S_XFER_TXS_START | I2S_XFER_RXS_START,
> + I2S_XFER_TXS_START | I2S_XFER_RXS_START);
> + if (ret < 0)
> + goto end;
> i2s->tx_start = true;
> } else {
> i2s->tx_start = false;
>
> - regmap_update_bits(i2s->regmap, I2S_DMACR,
> - I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_DISABLE);
> + ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
> + I2S_DMACR_TDE_ENABLE,
> + I2S_DMACR_TDE_DISABLE);
> + if (ret < 0)
> + goto end;
>
> if (!i2s->rx_start) {
> - regmap_update_bits(i2s->regmap, I2S_XFER,
> - I2S_XFER_TXS_START |
> - I2S_XFER_RXS_START,
> - I2S_XFER_TXS_STOP |
> - I2S_XFER_RXS_STOP);
> -
> + ret = regmap_update_bits(i2s->regmap, I2S_XFER,
> + I2S_XFER_TXS_START | I2S_XFER_RXS_START,
> + I2S_XFER_TXS_STOP | I2S_XFER_RXS_STOP);
> + if (ret < 0)
> + goto end;
> udelay(150);
> - regmap_update_bits(i2s->regmap, I2S_CLR,
> - I2S_CLR_TXC | I2S_CLR_RXC,
> - I2S_CLR_TXC | I2S_CLR_RXC);
> -
> + ret = regmap_update_bits(i2s->regmap, I2S_CLR,
> + I2S_CLR_TXC | I2S_CLR_RXC,
> + I2S_CLR_TXC | I2S_CLR_RXC);
> + if (ret < 0)
> + goto end;
> regmap_read(i2s->regmap, I2S_CLR, &val);
>
> /* Should wait for clear operation to finish */
> @@ -138,42 +175,55 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
> }
> }
> }
> +end:
> spin_unlock(&i2s->lock);
> + if (ret < 0)
> + dev_err(i2s->dev, "lrclk update failed\n");
> +
> + return ret;
> }
>
> -static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
> +static int rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
> {
> unsigned int val = 0;
> int retry = 10;
> + int ret = 0;
>
> spin_lock(&i2s->lock);
> if (on) {
> - regmap_update_bits(i2s->regmap, I2S_DMACR,
> - I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_ENABLE);
> -
> - regmap_update_bits(i2s->regmap, I2S_XFER,
> - I2S_XFER_TXS_START | I2S_XFER_RXS_START,
> - I2S_XFER_TXS_START | I2S_XFER_RXS_START);
> -
> + ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
> + I2S_DMACR_RDE_ENABLE,
> + I2S_DMACR_RDE_ENABLE);
> + if (ret < 0)
> + goto end;
> +
> + ret = regmap_update_bits(i2s->regmap, I2S_XFER,
> + I2S_XFER_TXS_START | I2S_XFER_RXS_START,
> + I2S_XFER_TXS_START | I2S_XFER_RXS_START);
> + if (ret < 0)
> + goto end;
> i2s->rx_start = true;
> } else {
> i2s->rx_start = false;
>
> - regmap_update_bits(i2s->regmap, I2S_DMACR,
> - I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_DISABLE);
> + ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
> + I2S_DMACR_RDE_ENABLE,
> + I2S_DMACR_RDE_DISABLE);
> + if (ret < 0)
> + goto end;
>
> if (!i2s->tx_start) {
> - regmap_update_bits(i2s->regmap, I2S_XFER,
> - I2S_XFER_TXS_START |
> - I2S_XFER_RXS_START,
> - I2S_XFER_TXS_STOP |
> - I2S_XFER_RXS_STOP);
> -
> + ret = regmap_update_bits(i2s->regmap, I2S_XFER,
> + I2S_XFER_TXS_START | I2S_XFER_RXS_START,
> + I2S_XFER_TXS_STOP | I2S_XFER_RXS_STOP);
> + if (ret < 0)
> + goto end;
> udelay(150);
> - regmap_update_bits(i2s->regmap, I2S_CLR,
> - I2S_CLR_TXC | I2S_CLR_RXC,
> - I2S_CLR_TXC | I2S_CLR_RXC);
> -
> + ret = regmap_update_bits(i2s->regmap, I2S_CLR,
> + I2S_CLR_TXC | I2S_CLR_RXC,
> + I2S_CLR_TXC | I2S_CLR_RXC);
> + if (ret < 0)
> + goto end;
> regmap_read(i2s->regmap, I2S_CLR, &val);
>
> /* Should wait for clear operation to finish */
> @@ -187,7 +237,12 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
> }
> }
> }
> +end:
> spin_unlock(&i2s->lock);
> + if (ret < 0)
> + dev_err(i2s->dev, "lrclk update failed\n");
> +
> + return ret;
> }
>
> static int rockchip_i2s_set_fmt(struct snd_soc_dai *cpu_dai,
> @@ -425,17 +480,25 @@ static int rockchip_i2s_trigger(struct snd_pcm_substream *substream,
> case SNDRV_PCM_TRIGGER_RESUME:
> case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> - rockchip_snd_rxctrl(i2s, 1);
> + ret = rockchip_snd_rxctrl(i2s, 1);
> else
> - rockchip_snd_txctrl(i2s, 1);
> + ret = rockchip_snd_txctrl(i2s, 1);
> + if (ret < 0)
> + return ret;
> + i2s_pinctrl_select_bclk_on(i2s);
> break;
> case SNDRV_PCM_TRIGGER_SUSPEND:
> case SNDRV_PCM_TRIGGER_STOP:
> case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> - if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> - rockchip_snd_rxctrl(i2s, 0);
> - else
> - rockchip_snd_txctrl(i2s, 0);
> + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> + if (!i2s->tx_start)
> + i2s_pinctrl_select_bclk_off(i2s);
> + ret = rockchip_snd_rxctrl(i2s, 0);
> + } else {
> + if (!i2s->rx_start)
> + i2s_pinctrl_select_bclk_off(i2s);
> + ret = rockchip_snd_txctrl(i2s, 0);
> + }
> break;
> default:
> ret = -EINVAL;
> @@ -736,6 +799,20 @@ static int rockchip_i2s_probe(struct platform_device *pdev)
> }
>
> i2s->bclk_ratio = 64;
> + i2s->pinctrl = devm_pinctrl_get(&pdev->dev);
> + if (IS_ERR(i2s->pinctrl))
> + dev_err(&pdev->dev, "failed to find i2s pinctrl\n");
> +
> + i2s->bclk_on = pinctrl_lookup_state(i2s->pinctrl, "bclk_on");
This will crash on any I2S controller that doesn't have pinctrl properties,
notably the I2S controller that sends audio to the HDMI controller will
never have pinctrl properties, since the connection is internal to the
SoC.
I'd say just put pinctrl lookup calls in an else block.
ChenYu
> + if (!IS_ERR_OR_NULL(i2s->bclk_on)) {
> + i2s->bclk_off = pinctrl_lookup_state(i2s->pinctrl, "bclk_off");
> + if (IS_ERR_OR_NULL(i2s->bclk_off)) {
> + dev_err(&pdev->dev, "failed to find i2s bclk_off\n");
> + goto err_clk;
> + }
> + }
> +
> + i2s_pinctrl_select_bclk_off(i2s);
>
> dev_set_drvdata(&pdev->dev, i2s);
>
> --
> 2.36.1.476.g0c4daa206d-goog
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Powered by blists - more mailing lists