[Public] > -----Original Message----- > From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx> > Sent: Tuesday, January 31, 2023 10:01 > To: Mukunda, Vijendar <Vijendar.Mukunda@xxxxxxx>; > broonie@xxxxxxxxxx; vkoul@xxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx > Cc: Katragadda, Mastan <Mastan.Katragadda@xxxxxxx>; Dommati, Sunil- > kumar <Sunil-kumar.Dommati@xxxxxxx>; open list <linux- > kernel@xxxxxxxxxxxxxxx>; Hiregoudar, Basavaraj > <Basavaraj.Hiregoudar@xxxxxxx>; Takashi Iwai <tiwai@xxxxxxxx>; Liam > Girdwood <lgirdwood@xxxxxxxxx>; Nathan Chancellor > <nathan@xxxxxxxxxx>; Limonciello, Mario <Mario.Limonciello@xxxxxxx>; > kondaveeti, Arungopal <Arungopal.kondaveeti@xxxxxxx>; Sanyog Kale > <sanyog.r.kale@xxxxxxxxx>; Bard Liao <yung-chuan.liao@xxxxxxxxxxxxxxx>; > Saba Kareem, Syed <Syed.SabaKareem@xxxxxxx> > 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.