RE: [PATCH 01/19] ASoC: amd: ps: create platform devices based on acp config

[Limonciello, Mario]

[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.