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

[Mukunda,Vijendar]

On 01/02/23 06:21, Pierre-Louis Bossart wrote:
>>>>>>> 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.
> You're confusing Controller and Manager.
>
> A Manager is the same as a 'Link', the two terms are interchangeable. It
> makes no sense to refer to a link number for a manager because there is
> no such concept.
>
> Only a Controller can have multiple links or managers. And each
> Controller needs to be declared as an ACPI device if you want to use the
> DisCo properties.
>
> The Managers/Links are not described as ACPI devices, that's a
> regrettable design decision made in MIPI circles many moons ago, that's
> why in the Intel code we have to manually create auxiliary devices based
> on the 'mipi-sdw-master-count' property.
>
>> So the _ADR really is identical for both.
Yes Controller has ACPI scope. Under controller based on
"mipi-sdw-manager-list" property manager instances will be created.
Manager and Link terms are interchangeable.

Below is the sample DSDT file if we go with two ACPI companion
devices.

Scope (\_SB.ACP)
    {

        Device (SWC0)
        {
            Name (_ADR, 0x05)  // _ADR: Address
        Name(_DSD, Package() {
                                        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                                        Package () {
                                        Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
                                        Package (2) {"mipi-sdw-manager-list", 1}, // v 1.0
                                        },
                                        ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), // Hierarchical Extension
                                        Package () {
                                        Package (2) {"mipi-sdw-link-0-subproperties", "SWM0"},
                                        }
                                        }) // End _DSD
        Name(SWM0, Package() {
                                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                                Package () {
                                Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
                                // ... place holder for SWM0 additional properties
                                }
                                }) // End SWM0.SWM

    Device (SLV0) { // SoundWire Slave 0
                        Name(_ADR, 0x000032025D131601)
                  } // END SLV0   

        } // END SWC0

     Device (SWC1)
        {
            Name (_ADR, 0x09)  // _ADR: Address
        Name(_DSD, Package() {
                                        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                                        Package () {
                                        Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
                                        Package (2) {"mipi-sdw-manager-list", 1},
                                        },
                                        ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
                                        Package () {
                                        Package (2) {"mipi-sdw-link-0-subproperties", "SWM0"},
                                      
                                        }
                                        }) // End _DSD
        Name(SWM0, Package() {
                                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                                Package () {
                                Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
                                // ... place holder for SWM0 additional properties
                                }
                                }) // End SWM0.SWM

    Device (SLV0) { // SoundWire Slave 0
                        Name(_ADR, 0x000032025D131601)
                  } // END SLV0

        } // END SWC1
    }
}



In above case, two manager instances will be created.
When manager under SWC1 scope tries to add peripheral
device, In sdw_slave_add() API its failing because peripheral
device descriptor uses link id followed by 48bit encoded address.
In above scenarios, both the manager's link id is zero only.



> That cannot possible work, even for Windows. You need to have a
> controller scope, and the _ADR can then be identical for different
> peripherals as long as this ADR is local to a controller scope.
>