| Message ID | 20190504002926.28815-2-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: corrections to ACPI and DisCo properties | expand |
On 03-05-19, 19:29, Pierre-Louis Bossart wrote: > The convention is that the SoundWire controller device is a child of > the HDAudio controller. However there can be more than one child > exposed in the DSDT table, and the current namespace walk returns the > last device. > > Add a filter and terminate early when a valid _ADR is provided, > otherwise keep iterating to find the next child. So what are the other devices in DSDT here.. > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/intel_init.c | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c > index d3d6b54c5791..f85db67d05f0 100644 > --- a/drivers/soundwire/intel_init.c > +++ b/drivers/soundwire/intel_init.c > @@ -150,6 +150,12 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, > { > struct sdw_intel_res *res = cdata; > struct acpi_device *adev; > + acpi_status status; > + u64 adr; > + > + status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr); > + if (ACPI_FAILURE(status)) > + return AE_OK; /* keep going */ > > if (acpi_bus_get_device(handle, &adev)) { > pr_err("%s: Couldn't find ACPI handle\n", __func__); > @@ -157,7 +163,18 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, > } > > res->handle = handle; > - return AE_OK; > + > + /* > + * On some Intel platforms, multiple children of the HDAS > + * device can be found, but only one of them is the SoundWire > + * controller. The SNDW device is always exposed with > + * Name(_ADR, 0x40000000) so filter accordingly > + */ > + if (adr != 0x40000000) I do not recall if 4 corresponds to the links you have or soundwire device type, is this number documented somewhere is HDA specs? Also it might good to create a define for this > + return AE_OK; /* keep going */ > + > + /* device found, stop namespace walk */ > + return AE_CTRL_TERMINATE; > } > > /** > -- > 2.17.1
On 5/7/19 7:26 AM, Vinod Koul wrote: > On 03-05-19, 19:29, Pierre-Louis Bossart wrote: >> The convention is that the SoundWire controller device is a child of >> the HDAudio controller. However there can be more than one child >> exposed in the DSDT table, and the current namespace walk returns the >> last device. >> >> Add a filter and terminate early when a valid _ADR is provided, >> otherwise keep iterating to find the next child. > > So what are the other devices in DSDT here.. this is what I see: Scope (HDAS) { Device (IDA) { Name (_ADR, 0x00020001) // _ADR: Address } } I thought this was nonsense but your question triggered me to look into the Intel SST ACPI specs (not public I am afraid but shared with the OS who shall not be named). Using the same source of information as below, I *believe* this is HDaudio related, bits 31..16 mean HDaudio with codec SDI 2, and NodeId 1 for the function group. This would make sense as I believe there are two codecs on the board that can be pin-strapped to boot either in HDaudio or SoundWire mode- but this is a conjecture only. At any rate, we need a hardware rework and mutual exclusion between HDaudio and SoundWire, so we have to ignore this one when SoundWire is enabled. >> + >> + /* >> + * On some Intel platforms, multiple children of the HDAS >> + * device can be found, but only one of them is the SoundWire >> + * controller. The SNDW device is always exposed with >> + * Name(_ADR, 0x40000000) so filter accordingly >> + */ >> + if (adr != 0x40000000) > > I do not recall if 4 corresponds to the links you have or soundwire > device type, is this number documented somewhere is HDA specs? I thought it was a magic number, but I did check and for once it's documented and the values match the spec :-) I see in the ACPI docs bits 31..28 set to 4 indicate a SoundWire Link Type and bits 3..0 indicate the SoundWire controller instance, the rest is reserved to zero. > > Also it might good to create a define for this I will respin this one to add the documentation above, and only filter on the 4 ms-bits. Thanks for forcing me to RTFM :-)
On 07-05-19, 09:43, Pierre-Louis Bossart wrote: > > > On 5/7/19 7:26 AM, Vinod Koul wrote: > > On 03-05-19, 19:29, Pierre-Louis Bossart wrote: > > > The convention is that the SoundWire controller device is a child of > > > the HDAudio controller. However there can be more than one child > > > exposed in the DSDT table, and the current namespace walk returns the > > > last device. > > > > > > Add a filter and terminate early when a valid _ADR is provided, > > > otherwise keep iterating to find the next child. > > > > So what are the other devices in DSDT here.. > > this is what I see: > > Scope (HDAS) > { > Device (IDA) > { > Name (_ADR, 0x00020001) // _ADR: Address > } > } > > I thought this was nonsense but your question triggered me to look into the > Intel SST ACPI specs (not public I am afraid but shared with the OS who > shall not be named). > Using the same source of information as below, I *believe* this is HDaudio > related, bits 31..16 mean HDaudio with codec SDI 2, and NodeId 1 for the > function group. This would make sense as I believe there are two codecs on > the board that can be pin-strapped to boot either in HDaudio or SoundWire > mode- but this is a conjecture only. > > At any rate, we need a hardware rework and mutual exclusion between HDaudio > and SoundWire, so we have to ignore this one when SoundWire is enabled. That is how I was expecting it to be... > > > + /* > > > + * On some Intel platforms, multiple children of the HDAS > > > + * device can be found, but only one of them is the SoundWire > > > + * controller. The SNDW device is always exposed with > > > + * Name(_ADR, 0x40000000) so filter accordingly > > > + */ > > > + if (adr != 0x40000000) > > > > I do not recall if 4 corresponds to the links you have or soundwire > > device type, is this number documented somewhere is HDA specs? > > I thought it was a magic number, but I did check and for once it's > documented and the values match the spec :-) > I see in the ACPI docs bits 31..28 set to 4 indicate a SoundWire Link Type > and bits 3..0 indicate the SoundWire controller instance, the rest is > reserved to zero. So in that case we should mask with bits 31..28 and match, who knows you may have multiple controller instances in future I had a vague recollection that this was documented in the spec, glad that in turned out to be the case. Btw was the update to HDA spec made public? > > Also it might good to create a define for this > > I will respin this one to add the documentation above, and only filter on > the 4 ms-bits. Thanks for forcing me to RTFM :-)
>>>> + /* >>>> + * On some Intel platforms, multiple children of the HDAS >>>> + * device can be found, but only one of them is the SoundWire >>>> + * controller. The SNDW device is always exposed with >>>> + * Name(_ADR, 0x40000000) so filter accordingly >>>> + */ >>>> + if (adr != 0x40000000) >>> >>> I do not recall if 4 corresponds to the links you have or soundwire >>> device type, is this number documented somewhere is HDA specs? >> >> I thought it was a magic number, but I did check and for once it's >> documented and the values match the spec :-) >> I see in the ACPI docs bits 31..28 set to 4 indicate a SoundWire Link Type >> and bits 3..0 indicate the SoundWire controller instance, the rest is >> reserved to zero. > > So in that case we should mask with bits 31..28 and match, who knows you > may have multiple controller instances in future yes, I was planning on only using the link type. > I had a vague recollection that this was documented in the spec, glad > that in turned out to be the case. > > Btw was the update to HDA spec made public? Not that I know of. The previous NHLT public doc has actually disappeared from the Intel site and I can't find it any longer, so currently the amount of public documentation is trending to zero :-( > >>> Also it might good to create a define for this >> >> I will respin this one to add the documentation above, and only filter on >> the 4 ms-bits. Thanks for forcing me to RTFM :-) >
On 08-05-19, 11:20, Pierre-Louis Bossart wrote: > > > > > > + /* > > > > > + * On some Intel platforms, multiple children of the HDAS > > > > > + * device can be found, but only one of them is the SoundWire > > > > > + * controller. The SNDW device is always exposed with > > > > > + * Name(_ADR, 0x40000000) so filter accordingly > > > > > + */ > > > > > + if (adr != 0x40000000) > > > > > > > > I do not recall if 4 corresponds to the links you have or soundwire > > > > device type, is this number documented somewhere is HDA specs? > > > > > > I thought it was a magic number, but I did check and for once it's > > > documented and the values match the spec :-) > > > I see in the ACPI docs bits 31..28 set to 4 indicate a SoundWire Link Type > > > and bits 3..0 indicate the SoundWire controller instance, the rest is > > > reserved to zero. > > > > So in that case we should mask with bits 31..28 and match, who knows you > > may have multiple controller instances in future > > yes, I was planning on only using the link type. > > > I had a vague recollection that this was documented in the spec, glad > > that in turned out to be the case. > > > > Btw was the update to HDA spec made public? > > Not that I know of. The previous NHLT public doc has actually disappeared > from the Intel site and I can't find it any longer, so currently the amount > of public documentation is trending to zero :-( > > > > > > > Also it might good to create a define for this > > > > > > I will respin this one to add the documentation above, and only filter on > > > the 4 ms-bits. Thanks for forcing me to RTFM :-) Yeah about that someone was indeed complaining about that on IRC, it is shame that link is valid but doc is gone... check with Rakesh or someone they might have a copy...
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index d3d6b54c5791..f85db67d05f0 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -150,6 +150,12 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, { struct sdw_intel_res *res = cdata; struct acpi_device *adev; + acpi_status status; + u64 adr; + + status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr); + if (ACPI_FAILURE(status)) + return AE_OK; /* keep going */ if (acpi_bus_get_device(handle, &adev)) { pr_err("%s: Couldn't find ACPI handle\n", __func__); @@ -157,7 +163,18 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, } res->handle = handle; - return AE_OK; + + /* + * On some Intel platforms, multiple children of the HDAS + * device can be found, but only one of them is the SoundWire + * controller. The SNDW device is always exposed with + * Name(_ADR, 0x40000000) so filter accordingly + */ + if (adr != 0x40000000) + return AE_OK; /* keep going */ + + /* device found, stop namespace walk */ + return AE_CTRL_TERMINATE; } /**
The convention is that the SoundWire controller device is a child of the HDAudio controller. However there can be more than one child exposed in the DSDT table, and the current namespace walk returns the last device. Add a filter and terminate early when a valid _ADR is provided, otherwise keep iterating to find the next child. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/soundwire/intel_init.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)