Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
To:     Brent Lu <brent.lu@intel.com>,
        "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>
CC:     Support Opensource <Support.Opensource@diasemi.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "cychiang@google.com" <cychiang@google.com>,
        "mac.chiang@intel.com" <mac.chiang@intel.com>
Subject: RE: [PATCH] ASoC: da7219: check SRM lock in trigger callback
Thread-Topic: [PATCH] ASoC: da7219: check SRM lock in trigger callback
Thread-Index: AQHV3+t05QGFzNuez06VZ5VdF8K2kqgUfAmA
Date:   Mon, 10 Feb 2020 14:32:05 +0000
Message-ID: <AM6PR10MB2263F302A86B17C95B16361280190@AM6PR10MB2263.EURPRD10.PROD.OUTLOOK.COM>
References: <1581322611-25695-1-git-send-email-brent.lu@intel.com>
In-Reply-To: <1581322611-25695-1-git-send-email-brent.lu@intel.com>
Accept-Language: en-GB, en-US
Content-Language: en-US
x-ms-exchange-messagesentrepresentingtype: 1
x-originating-ip: [165.225.80.85]
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: bda9da1c-915e-49b6-ff41-08d7ae3600c1
x-ms-traffictypediagnostic: AM6PR10MB2677:
x-ms-exchange-sharedmailbox-routingagent-processed: True
x-ms-exchange-transport-forked: True
x-microsoft-antispam-prvs: <AM6PR10MB26774C2738A2E21368F4EB5AA7190@AM6PR10MB2677.EURPRD10.PROD.OUTLOOK.COM>
x-ms-oob-tlc-oobclassifiers: OLM:7691;
x-forefront-prvs: 03094A4065
x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(4636009)(346002)(376002)(39850400004)(136003)(366004)(396003)(199004)(189003)(81156014)(110136005)(54906003)(81166006)(66556008)(7696005)(76116006)(33656002)(66476007)(66946007)(8676002)(64756008)(66446008)(55236004)(8936002)(26005)(53546011)(186003)(6506007)(2906002)(52536014)(478600001)(316002)(86362001)(71200400001)(55016002)(9686003)(5660300002)(4326008);DIR:OUT;SFP:1101;SCL:1;SRVR:AM6PR10MB2677;H:AM6PR10MB2263.EURPRD10.PROD.OUTLOOK.COM;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:0;MX:1;
received-spf: None (protection.outlook.com: diasemi.com does not designate
 permitted sender hosts)
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam: BCL:0;
x-microsoft-antispam-message-info: c6Nd8AvOJqNONXgjX1w1NU9nS6DqoTDT5TIyK71Wq14Kzcjm2Xq4C6SNRmjiBLHETjlKhgjNr/5zSu9qPBL0AuCk0+2gP6k2aR/XkGlKiLY+zCx4vPyIQWbJW7Ebc2Z61FXULWt99ERQVaZjZHwoyqPgSCyzXcJ+z/awfm70HFh3PyA3Ho3+k+qCWbzculJgerXrXHwHGKN807vKlEq3SJViJqdxWoxk2awcDJYu5Q/DpeYvDxRzLquKgYgEw0d67ulUkT0KuFp+rEJuCzFasGd+1S0USTrJv1oTv6HBgj4RypWV3kWGzy+IQBX6Iv/gFJpEd4qJsXhOp4aU556DlvFulnJiL3Al+DVVfd9qc1GHUZjy1AlDY9RnoMrVUXewhpH3PNqWvIy0of0rJYjtufyFtqevdRcjXYkIqbb4JEN1oemHY55gxfjqFWmRdnDU
x-ms-exchange-antispam-messagedata: vGRd/gZSr9BVrSpaopthSWO+/uRRTnzOhM3UJoJfkYa9IxyknByNzqHmcdWqy7F1SCKe3KqcOiJqIfbcooCQgPpmvji9ZgP+oumAS14XMISLlEjqcuBd1D9eXr08fYjgbB+HICA9gmhaO/IB1+RxgA==
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: diasemi.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bda9da1c-915e-49b6-ff41-08d7ae3600c1
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Feb 2020 14:32:06.0035
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 511e3c0e-ee96-486e-a2ec-e272ffa37b7c
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Pj8ErrMpFarKdjgWExjkTPyFlC+84Cmssn9+an/rP6MPIj+EhF6HFYJ5h2teD+HrQIT1COX9k/b/6Fta4OBq5YPhH0GLT4A0gtzqgiyUHO8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR10MB2677
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 10 February 2020 08:17, Brent Lu wrote:

> Intel sst firmware turns on BCLK/WCLK in START Ioctl call which timing is
> later than the DAPM SUPPLY event handler da7219_dai_event is called (in
> PREPARED state). Therefore, the SRM lock check always fail.
>=20
> Moving the check to trigger callback could ensure the SRM is locked befor=
e
> DSP starts to process data and avoid possisble noise.

Could ensure? This change seems specific to Intel DSP based systems, at lea=
st
from the description. Having looked through the core, the trigger code for =
a
codec is seemingly always called before the trigger for the CPU. How will t=
his
work for other platforms, assuming their clocks are enabled in the CPU DAI
trigger function by default?

Can we always guarantee the CPU side isn't going to send anything other tha=
n 0s
until after SRM has locked?

>=20
> Signed-off-by: Brent Lu <brent.lu@intel.com>
> ---
>  sound/soc/codecs/da7219.c | 68 +++++++++++++++++++++++++++++++--------
> --------
>  1 file changed, 45 insertions(+), 23 deletions(-)
>=20
> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
> index f83a6ea..0fb5ea5 100644
> --- a/sound/soc/codecs/da7219.c
> +++ b/sound/soc/codecs/da7219.c
> @@ -794,9 +794,7 @@ static int da7219_dai_event(struct snd_soc_dapm_widge=
t
> *w,
>  	struct snd_soc_component *component =3D
> snd_soc_dapm_to_component(w->dapm);
>  	struct da7219_priv *da7219 =3D
> snd_soc_component_get_drvdata(component);
>  	struct clk *bclk =3D da7219->dai_clks[DA7219_DAI_BCLK_IDX];
> -	u8 pll_ctrl, pll_status;
> -	int i =3D 0, ret;
> -	bool srm_lock =3D false;
> +	int ret;
>=20
>  	switch (event) {
>  	case SND_SOC_DAPM_PRE_PMU:
> @@ -820,26 +818,6 @@ static int da7219_dai_event(struct
> snd_soc_dapm_widget *w,
>  		/* PC synchronised to DAI */
>  		snd_soc_component_update_bits(component,
> DA7219_PC_COUNT,
>  				    DA7219_PC_FREERUN_MASK, 0);
> -
> -		/* Slave mode, if SRM not enabled no need for status checks */
> -		pll_ctrl =3D snd_soc_component_read32(component,
> DA7219_PLL_CTRL);
> -		if ((pll_ctrl & DA7219_PLL_MODE_MASK) !=3D
> DA7219_PLL_MODE_SRM)
> -			return 0;
> -
> -		/* Check SRM has locked */
> -		do {
> -			pll_status =3D snd_soc_component_read32(component,
> DA7219_PLL_SRM_STS);
> -			if (pll_status & DA7219_PLL_SRM_STS_SRM_LOCK) {
> -				srm_lock =3D true;
> -			} else {
> -				++i;
> -				msleep(50);
> -			}
> -		} while ((i < DA7219_SRM_CHECK_RETRIES) && (!srm_lock));
> -
> -		if (!srm_lock)
> -			dev_warn(component->dev, "SRM failed to lock\n");
> -
>  		return 0;
>  	case SND_SOC_DAPM_POST_PMD:
>  		/* PC free-running */
> @@ -1658,12 +1636,56 @@ static int da7219_hw_params(struct
> snd_pcm_substream *substream,
>  	return 0;
>  }
>=20
> +static int da7219_set_dai_trigger(struct snd_pcm_substream *substream, i=
nt
> cmd,
> +				  struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component =3D dai->component;
> +	u8 pll_ctrl, pll_status;
> +	int i =3D 0;
> +	bool srm_lock =3D false;
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +		/* Slave mode, if SRM not enabled no need for status checks */
> +		pll_ctrl =3D snd_soc_component_read32(component,
> DA7219_PLL_CTRL);

I was under the impression that 'trigger()' was atomic? We'd have to have
some kind of workqueue to do all of this, which means we'd almost certainly=
 lose
some PCM data at the start of a stream.

> +		if ((pll_ctrl & DA7219_PLL_MODE_MASK) !=3D
> DA7219_PLL_MODE_SRM)
> +			return 0;
> +
> +		/* Check SRM has locked */
> +		do {
> +			pll_status =3D snd_soc_component_read32(component,
> +							DA7219_PLL_SRM_STS);
> +			if (pll_status & DA7219_PLL_SRM_STS_SRM_LOCK) {
> +				srm_lock =3D true;
> +			} else {
> +				++i;
> +				msleep(50);
> +			}
> +		} while ((i < DA7219_SRM_CHECK_RETRIES) && (!srm_lock));
> +
> +		if (!srm_lock)
> +			dev_warn(component->dev, "SRM failed to lock\n");
> +
> +		break;
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct snd_soc_dai_ops da7219_dai_ops =3D {
>  	.hw_params	=3D da7219_hw_params,
>  	.set_sysclk	=3D da7219_set_dai_sysclk,
>  	.set_pll	=3D da7219_set_dai_pll,
>  	.set_fmt	=3D da7219_set_dai_fmt,
>  	.set_tdm_slot	=3D da7219_set_dai_tdm_slot,
> +	.trigger	=3D da7219_set_dai_trigger,
>  };
>=20
>  #define DA7219_FORMATS (SNDRV_PCM_FMTBIT_S16_LE |
> SNDRV_PCM_FMTBIT_S20_3LE |\
> --
> 2.7.4