enable CometLake rt5682 evaluate board by zhuyingjiang · Pull Request #535 · thesofproject/linux

zhuyingjiang · 2019-01-14T06:49:38Z

The CometLake use a rt5682 codec, which has analog pipeline, DMIC
and HDMI pipelines. This is the machine driver for it. Also this
machine driver can be modified to the intel generic machine driver
for RVP with rt5682.
This fix #514
Signed-off-by: Zhu Yingjiang yingjiang.zhu@linux.intel.com

The CometLake use a rt5682 codec, which has analog pipeline, DMIC and HDMI pipelines. This is the machine driver for it. Also this machine driver can be modified to the intel generic machine driver for RVP with rt5682. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

bardliao

Please remove platform specific naming such as cml, cometlake... and rename it to a generic name if the driver is going to be used for different platforms. How about "sof_rt5682"?

bardliao · 2019-01-14T07:01:54Z

+	CML_DPCM_AUDIO_HDMI1_PB,
+	CML_DPCM_AUDIO_HDMI2_PB,
+	CML_DPCM_AUDIO_HDMI3_PB,
+};


What does this enum for?

this is all copy/paste stuff that's irrelevant for SOF. please only add what you actually test.

bardliao · 2019-01-14T08:13:38Z

+	.owner = THIS_MODULE,
+	.dai_link = cometlake_dais,
+	.num_links = ARRAY_SIZE(cometlake_dais),
+	.fully_routed = false,


I would prefer to add dapm_routes and set fully_routed = true.

bardliao · 2019-01-14T08:26:23Z

+		.cpu_dai_name = "iDisp1 Pin",
+		.codec_name = "ehdaudio0D2",
+		.codec_dai_name = "intel-hdmi-hifi1",
+		.platform_name = "0000:00:05.0",


Is the platform_name correct? Can we just set it to "sof-audio" if the machine driver is only used by SOF?

bardliao · 2019-01-14T08:26:34Z

+		.be_hw_params_fixup = cometlake_dmic_fixup,
+		.dpcm_capture = 1,
+		.no_pcm = 1,
+	},


Should we add "#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)" if the machine driver is going to be used for different platforms? So that we can use it in a platform without HDMI supported.

we need to figure out the usage of platform_name, it's inconsistent and caused issues discussed on alsa-devel

bardliao · 2019-01-14T08:37:12Z

+	ret = snd_soc_card_jack_new(rtd->card, "Headset Jack",
+			SND_JACK_HEADSET | SND_JACK_BTN_0 | SND_JACK_BTN_1 |
+			SND_JACK_BTN_2 | SND_JACK_BTN_3 | SND_JACK_LINEOUT,
+			&ctx->cometlake_headset, NULL, 0);


Do we need SND_JACK_LINEOUT? It seems there is no Line out on rt5682 codec,

bardliao · 2019-01-14T08:37:19Z

+		dev_err(rtd->dev, "set TDM slot err:%d\n", ret);
+		return ret;
+	}
+


If we need to support different sampling rate, we will need to set pll here.

this is useless for SOF.

@plbossart Sorry, did you mean snd_soc_dai_set_tdm_slot() is useless? It is embarrassed that we need to call snd_soc_dai_set_tdm_slot() to set the slot width (channel length) of rt5682 even we don't use tdm.

sorry, please ignore my comment, I was confused with some old SST code where the set_tdm_slot if used for the cpu_dai. This should be the same as in the existing glk_rt5682....c

bardliao · 2019-01-14T08:38:04Z

+	/*
+	 * set BE channel constraint as user FE channels
+	 */
+	channels->min = channels->max = 4;


Does it necessary to fix the channels to 4?

zhuyingjiang · 2019-01-14T10:21:30Z

Please remove platform specific naming such as cml, cometlake... and rename it to a generic name if the driver is going to be used for different platforms. How about "sof_rt5682"?

How about "intel_rvp_rt5682", or "sof_rvp_rt5682" @bardliao @mengdonglin

plbossart

Please remove all the parts that are cAVS-specific. We only care about SOF at this point.
Also please make this generic so that we can reuse this machine driver on other development platforms. Nothing in there is CML specific - except for the clock settings.

plbossart · 2019-01-14T17:58:47Z


 endif ## SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC || SND_SOC_SOF_HDA_AUDIO_CODEC

+if SND_SOC_INTEL_CNL || (SND_SOC_SOF_CANNONLAKE && SND_SOC_SOF_HDA_LINK)


we don't test the cAVS driver so this should be SOF only (same as PCM512x)

plbossart · 2019-01-14T17:59:54Z


+if SND_SOC_INTEL_CNL || (SND_SOC_SOF_CANNONLAKE && SND_SOC_SOF_HDA_LINK)
+config SND_SOC_INTEL_CML_RT5682_MACH
+	tristate "Cometlake with rt5682 I2S mode"


what's specific about CML? We could be using this driver on other platforms, and in fact there is a plan to use rt5682 as a replacement for rt5651 so could be using this on up2 as well.

plbossart · 2019-01-14T18:00:47Z

+	CML_DPCM_AUDIO_HDMI1_PB,
+	CML_DPCM_AUDIO_HDMI2_PB,
+	CML_DPCM_AUDIO_HDMI3_PB,
+};


this is all copy/paste stuff that's irrelevant for SOF. please only add what you actually test.

plbossart · 2019-01-14T18:01:02Z

+
+	/* set SSP to 24 bit */
+	snd_mask_none(fmt);
+	snd_mask_set(fmt, SNDRV_PCM_FORMAT_S24_LE);


snd_mask_set_format

plbossart · 2019-01-14T18:01:21Z

+		dev_err(rtd->dev, "set TDM slot err:%d\n", ret);
+		return ret;
+	}
+


this is useless for SOF.

plbossart · 2019-01-14T18:03:23Z

+		.init = cometlake_rt5682_codec_init,
+		.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
+			SND_SOC_DAIFMT_CBS_CFS,
+		.ignore_pmdown_time = 1,


the format is specific by topology in SOF

plbossart · 2019-01-14T18:03:59Z

+		.be_hw_params_fixup = cometlake_dmic_fixup,
+		.dpcm_capture = 1,
+		.no_pcm = 1,
+	},


we need to figure out the usage of platform_name, it's inconsistent and caused issues discussed on alsa-devel

plbossart · 2019-01-14T18:04:46Z

 	.chip_info = &cnl_chip_info,
 	.nocodec_fw_filename = "intel/sof-cnl.ri",
-	.nocodec_tplg_filename = "intel/sof-cnl.tplg",
+	.nocodec_tplg_filename = "intel/sof-cnl-nocodec.tplg",


separate PR

wenqingfu · 2019-01-14T19:13:47Z

How about "intel_rvp_rt5682", or "sof_rvp_rt5682"

there is no need for 'rvp' in the middle, it's meaningless to people who don't know what it stands for.

plbossart · 2019-01-14T19:48:33Z

maybe all machine drivers supporting SOF only should really be tagged as such with sof_xyz, where xyz is based on codec/speaker amp hints. We'd need however to make sure HDMI can be commented out to make sure it's reusable even with legacy platforms where HDMI does not exist (as done for pcm512x)

zhuyingjiang · 2019-01-18T12:20:48Z

Close this since bard opened #553

Move all allocations outside of the regulator_lock()ed section. ====================================================== WARNING: possible circular locking dependency detected 5.7.13+ #535 Not tainted ------------------------------------------------------ f2fs_discard-179:7/702 is trying to acquire lock: c0e5d920 (regulator_list_mutex){+.+.}-{3:3}, at: regulator_lock_dependent+0x54/0x2c0 but task is already holding lock: cb95b080 (&dcc->cmd_lock){+.+.}-{3:3}, at: __issue_discard_cmd+0xec/0x5f8 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: [...] -> #3 (fs_reclaim){+.+.}-{0:0}: fs_reclaim_acquire.part.11+0x40/0x50 fs_reclaim_acquire+0x24/0x28 __kmalloc_track_caller+0x54/0x218 kstrdup+0x40/0x5c create_regulator+0xf4/0x368 regulator_resolve_supply+0x1a0/0x200 regulator_register+0x9c8/0x163c [...] other info that might help us debug this: Chain exists of: regulator_list_mutex --> &sit_i->sentry_lock --> &dcc->cmd_lock [...] Fixes: f8702f9 ("regulator: core: Use ww_mutex for regulators locking") Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> Cc: stable@vger.kernel.org Link: https://lore.kernel.org/r/6eebc99b2474f4ffaa0405b15178ece0e7e4f608.1597195321.git.mirq-linux@rere.qmqm.pl Signed-off-by: Mark Brown <broonie@kernel.org>

The altmode device release refers to its parent device, but without keeping a reference to it. When registering the altmode, get a reference to the parent and put it in the release function. Before this fix, when using CONFIG_DEBUG_KOBJECT_RELEASE, we see issues like this: [ 43.572860] kobject: 'port0.0' (ffff8880057ba008): kobject_release, parent 0000000000000000 (delayed 3000) [ 43.573532] kobject: 'port0.1' (ffff8880057bd008): kobject_release, parent 0000000000000000 (delayed 1000) [ 43.574407] kobject: 'port0' (ffff8880057b9008): kobject_release, parent 0000000000000000 (delayed 3000) [ 43.575059] kobject: 'port1.0' (ffff8880057ca008): kobject_release, parent 0000000000000000 (delayed 4000) [ 43.575908] kobject: 'port1.1' (ffff8880057c9008): kobject_release, parent 0000000000000000 (delayed 4000) [ 43.576908] kobject: 'typec' (ffff8880062dbc00): kobject_release, parent 0000000000000000 (delayed 4000) [ 43.577769] kobject: 'port1' (ffff8880057bf008): kobject_release, parent 0000000000000000 (delayed 3000) [ 46.612867] ================================================================== [ 46.613402] BUG: KASAN: slab-use-after-free in typec_altmode_release+0x38/0x129 [ 46.614003] Read of size 8 at addr ffff8880057b9118 by task kworker/2:1/48 [ 46.614538] [ 46.614668] CPU: 2 UID: 0 PID: 48 Comm: kworker/2:1 Not tainted 6.12.0-rc1-00138-gedbae730ad31 thesofproject#535 [ 46.615391] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014 [ 46.616042] Workqueue: events kobject_delayed_cleanup [ 46.616446] Call Trace: [ 46.616648] <TASK> [ 46.616820] dump_stack_lvl+0x5b/0x7c [ 46.617112] ? typec_altmode_release+0x38/0x129 [ 46.617470] print_report+0x14c/0x49e [ 46.617769] ? rcu_read_unlock_sched+0x56/0x69 [ 46.618117] ? __virt_addr_valid+0x19a/0x1ab [ 46.618456] ? kmem_cache_debug_flags+0xc/0x1d [ 46.618807] ? typec_altmode_release+0x38/0x129 [ 46.619161] kasan_report+0x8d/0xb4 [ 46.619447] ? typec_altmode_release+0x38/0x129 [ 46.619809] ? process_scheduled_works+0x3cb/0x85f [ 46.620185] typec_altmode_release+0x38/0x129 [ 46.620537] ? process_scheduled_works+0x3cb/0x85f [ 46.620907] device_release+0xaf/0xf2 [ 46.621206] kobject_delayed_cleanup+0x13b/0x17a [ 46.621584] process_scheduled_works+0x4f6/0x85f [ 46.621955] ? __pfx_process_scheduled_works+0x10/0x10 [ 46.622353] ? hlock_class+0x31/0x9a [ 46.622647] ? lock_acquired+0x361/0x3c3 [ 46.622956] ? move_linked_works+0x46/0x7d [ 46.623277] worker_thread+0x1ce/0x291 [ 46.623582] ? __kthread_parkme+0xc8/0xdf [ 46.623900] ? __pfx_worker_thread+0x10/0x10 [ 46.624236] kthread+0x17e/0x190 [ 46.624501] ? kthread+0xfb/0x190 [ 46.624756] ? __pfx_kthread+0x10/0x10 [ 46.625015] ret_from_fork+0x20/0x40 [ 46.625268] ? __pfx_kthread+0x10/0x10 [ 46.625532] ret_from_fork_asm+0x1a/0x30 [ 46.625805] </TASK> [ 46.625953] [ 46.626056] Allocated by task 678: [ 46.626287] kasan_save_stack+0x24/0x44 [ 46.626555] kasan_save_track+0x14/0x2d [ 46.626811] __kasan_kmalloc+0x3f/0x4d [ 46.627049] __kmalloc_noprof+0x1bf/0x1f0 [ 46.627362] typec_register_port+0x23/0x491 [ 46.627698] cros_typec_probe+0x634/0xbb6 [ 46.628026] platform_probe+0x47/0x8c [ 46.628311] really_probe+0x20a/0x47d [ 46.628605] device_driver_attach+0x39/0x72 [ 46.628940] bind_store+0x87/0xd7 [ 46.629213] kernfs_fop_write_iter+0x1aa/0x218 [ 46.629574] vfs_write+0x1d6/0x29b [ 46.629856] ksys_write+0xcd/0x13b [ 46.630128] do_syscall_64+0xd4/0x139 [ 46.630420] entry_SYSCALL_64_after_hwframe+0x76/0x7e [ 46.630820] [ 46.630946] Freed by task 48: [ 46.631182] kasan_save_stack+0x24/0x44 [ 46.631493] kasan_save_track+0x14/0x2d [ 46.631799] kasan_save_free_info+0x3f/0x4d [ 46.632144] __kasan_slab_free+0x37/0x45 [ 46.632474] kfree+0x1d4/0x252 [ 46.632725] device_release+0xaf/0xf2 [ 46.633017] kobject_delayed_cleanup+0x13b/0x17a [ 46.633388] process_scheduled_works+0x4f6/0x85f [ 46.633764] worker_thread+0x1ce/0x291 [ 46.634065] kthread+0x17e/0x190 [ 46.634324] ret_from_fork+0x20/0x40 [ 46.634621] ret_from_fork_asm+0x1a/0x30 Fixes: 8a37d87 ("usb: typec: Bus type for alternate modes") Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Link: https://lore.kernel.org/r/20241004123738.2964524-1-cascardo@igalia.com Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

zhuyingjiang requested review from bardliao, lgirdwood, mengdonglin and plbossart January 14, 2019 06:50

bardliao requested changes Jan 14, 2019

View reviewed changes

plbossart approved these changes Jan 14, 2019

View reviewed changes

bardliao mentioned this pull request Jan 17, 2019

ASoC: intel: add sof rt5682 machine driver #553

Merged

zhuyingjiang closed this Jan 18, 2019


		endif ## SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC \|\| SND_SOC_SOF_HDA_AUDIO_CODEC

		if SND_SOC_INTEL_CNL \|\| (SND_SOC_SOF_CANNONLAKE && SND_SOC_SOF_HDA_LINK)

Conversation

zhuyingjiang commented Jan 14, 2019

Uh oh!

bardliao left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhuyingjiang commented Jan 14, 2019

Uh oh!

plbossart left a comment

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!

wenqingfu commented Jan 14, 2019

Uh oh!

plbossart commented Jan 14, 2019

Uh oh!

zhuyingjiang commented Jan 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants