[Public]
-----Original Message----- From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Sent: Tuesday, January 31, 2023 10:01 To: Mukunda, Vijendar Vijendar.Mukunda@amd.com; broonie@kernel.org; vkoul@kernel.org; alsa-devel@alsa-project.org Cc: Katragadda, Mastan Mastan.Katragadda@amd.com; Dommati, Sunil- kumar Sunil-kumar.Dommati@amd.com; open list <linux- kernel@vger.kernel.org>; Hiregoudar, Basavaraj Basavaraj.Hiregoudar@amd.com; Takashi Iwai tiwai@suse.com; Liam Girdwood lgirdwood@gmail.com; Nathan Chancellor nathan@kernel.org; Limonciello, Mario Mario.Limonciello@amd.com; kondaveeti, Arungopal Arungopal.kondaveeti@amd.com; Sanyog Kale sanyog.r.kale@intel.com; Bard Liao yung-chuan.liao@linux.intel.com; Saba Kareem, Syed Syed.SabaKareem@amd.com Subject: Re: [PATCH 01/19] ASoC: amd: ps: create platform devices based on acp config
On 1/31/23 07:09, Mukunda,Vijendar wrote:
On 16/01/23 13:32, Mukunda,Vijendar wrote:
On 13/01/23 22:41, Pierre-Louis Bossart wrote:
> + if (is_dmic_dev && is_sdw_dev) { > + switch (acp_data->sdw_master_count) { > + case 1: > + acp_data->pdev_mask =
ACP63_SDW_PDM_DEV_MASK;
> + acp_data->pdev_count =
ACP63_SDW0_PDM_MODE_DEVS;
> + break; > + case 2: > + acp_data->pdev_mask =
ACP63_SDW_PDM_DEV_MASK;
> + acp_data->pdev_count =
ACP63_SDW0_SDW1_PDM_MODE_DEVS;
> + break; so the cover letter is indeed wrong and confuses two controllers for
two
managers.
ACP IP has two independent manager instances driven by separate
controller
each which are connected in different power domains.
we should create two separate ACPI companion devices for separate manager instance. Currently we have limitations with BIOS. we are going with single ACPI companion device. We will update the changes later.
Humm, this is tricky. The BIOS interface isn't something that can be changed at will on the kernel side, you'd have to maintain two solutions with a means to detect which one to use.
Or is this is a temporary issue on development devices, then that part should probably not be upstreamed.
It's a temporary issue on development devices. We had discussion with Windows dev team and BIOS team. They have agreed to modify ACPI companion device logic. We will update the two companion devices logic for two manager instances in V2 version.
After experimenting, two ACPI companion devices approach, we got an update from Windows team, there is a limitation on windows stack. For current platform, we can't proceed with two ACPI companion devices.
so how would the two controllers be declared then in the DSDT used by Windows? There's a contradiction between having a single companion device and the ability to set the 'manager-number' to one.
You probably want to give an example of what you have, otherwise we probably will talk past each other.
Even on Linux side, if we create two ACPI companion devices followed by creating a single soundwire manager instance per Soundwire controller, we have observed an issue in a scenario, where similar codec parts(UID are also same) are connected on both soundwire manager instances.
We've been handling this case of two identical amplifiers on two different links for the last 3 years. I don't see how this could be a problem, the codecs are declared in the scope of the companion device and the _ADR defines in bits [51..48] which link the codec is connected to.
The problem is that there are two managers in the specified AMD design, and the codecs are both on "Link 0" for each manager.
So the _ADR really is identical for both.
see example below from a TigerLake device with two identical amsp on link 1 and 2.
Scope (_SB.PC00.HDAS.SNDW) { Device (SWD1) { Name (_ADR, 0x000131025D131601) // _ADR: Address
Device (SWD2) { Name (_ADR, 0x000230025D131601) // _ADR: Address
As per MIPI Disco spec, for single link controllers Link ID should be set to zero. If we use Link ID as zero, for the soundwire manager which is on the second soundwire controller ACPI device scope, then soundwire framework is not allowing to create peripheral device node as its duplicate one.
I still don't see how it's possible. There is an IDA used in the bus allocation
static int sdw_get_id(struct sdw_bus *bus) { int rc = ida_alloc(&sdw_bus_ida, GFP_KERNEL);
if (rc < 0) return rc;
bus->id = rc; return 0; }
and that's used for debugfs
/* create the debugfs master-N */ snprintf(name, sizeof(name), "master-%d-%d", bus->id, bus-
link_id);
as well as in sdw_master_device_add(): dev_set_name(&md->dev, "sdw-master-%d", bus->id);
can you clarify what part of the 'SoundWire framework' is problematic? I guess the problem is that you have identical devices with the same _ADR under the same manager, which is problematic indeed, but that's not a SoundWire framework issue, just not a supported configuration.
If we want to support two ACPI companion device approach on our future platforms, how to proceed?
Well how about dealing with a single companion device first, cause that's what you have now and that's already problematic.