[alsa-devel] ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device
Hi,
I have an Intel Bay Trail (BYT-T) tablet that was originally shipped with Android. With the right quirks, bytcr-rt5640 is working fine, but there is a problem in sst_acpi.c that is preventing it from working with a mainline kernel:
Even though this is a BYT-T device, there is no IRQ at index 5 in the ACPI DSDT table. This means that SST will fail to probe, and actually leads to a NULL pointer dereference later when the ALSA device is first opened. (I have submitted a possible solution for this as "[PATCH] ASoC: Intel: sst: Delay machine device creation until after initialization")
The correct IRQ is actually located on index 0, just like it is already being used for BYT-CR devices. So if I force is_byt_cr() to return TRUE, everything works as expected.
Here is the relevant part from the ACPI DSDT table:
Name (_ADR, Zero) // _ADR: Address Name (_HID, "80860F28" /* Intel SST Audio DSP */) // _HID: Hardware ID Name (_CID, "80860F28" /* Intel SST Audio DSP */) // _CID: Compatible ID Name (_DDN, "Intel(R) Low Power Audio Controller - 80860F28") // _DDN: DOS Device Name Name (_SUB, "80867270") // _SUB: Subsystem ID Name (_UID, One) // _UID: Unique ID
Name (RBUF, ResourceTemplate () { Memory32Fixed (ReadWrite, 0x12345678, // Address Base 0x00200000, // Address Length _Y08) Memory32Fixed (ReadWrite, 0xFE830000, // Address Base 0x00001000, // Address Length _Y09) Memory32Fixed (ReadWrite, 0x55AA55AA, // Address Base 0x00200000, // Address Length _Y0A) Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, ) { 0x0000001D, } })
Unlike many of the other DSDT dumps I've looked at, there is only one interrupt listed. Full ACPI DSDT table is at [1].
Since there is no IRQ at index 5, platform_get_irq will return -ENXIO. Couldn't we fall back to index 0 in this case? I would say that if the seemingly "correct" IRQ at index 5 does not even exist, we still have a better chance of picking the right one if we try the one at index 0. Or we could check the number of interrupts that are actually available.
The other option would be some kind of DMI-based quirk, but personally I would prefer to avoid that.. (In my opinion, there is way too much device specific code with the quirks etc already...)
Or do you have any other suggestions?
Thanks, Stephan
[1]: https://github.com/me176c-dev/me176c-acpi/blob/f48c78c11b0819b899f988407b6ec...
Hi,
On 16-12-18 19:54, Stephan Gerhold wrote:
Hi,
I have an Intel Bay Trail (BYT-T) tablet that was originally shipped with Android. With the right quirks, bytcr-rt5640 is working fine, but there is a problem in sst_acpi.c that is preventing it from working with a mainline kernel:
Even though this is a BYT-T device, there is no IRQ at index 5 in the ACPI DSDT table. This means that SST will fail to probe, and actually leads to a NULL pointer dereference later when the ALSA device is first opened. (I have submitted a possible solution for this as "[PATCH] ASoC: Intel: sst: Delay machine device creation until after initialization")
The correct IRQ is actually located on index 0, just like it is already being used for BYT-CR devices. So if I force is_byt_cr() to return TRUE, everything works as expected.
Here is the relevant part from the ACPI DSDT table:
Name (_ADR, Zero) // _ADR: Address Name (_HID, "80860F28" /* Intel SST Audio DSP */) // _HID: Hardware ID Name (_CID, "80860F28" /* Intel SST Audio DSP */) // _CID: Compatible ID Name (_DDN, "Intel(R) Low Power Audio Controller - 80860F28") // _DDN: DOS Device Name Name (_SUB, "80867270") // _SUB: Subsystem ID Name (_UID, One) // _UID: Unique ID
Name (RBUF, ResourceTemplate () { Memory32Fixed (ReadWrite, 0x12345678, // Address Base 0x00200000, // Address Length _Y08) Memory32Fixed (ReadWrite, 0xFE830000, // Address Base 0x00001000, // Address Length _Y09) Memory32Fixed (ReadWrite, 0x55AA55AA, // Address Base 0x00200000, // Address Length _Y0A) Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, ) { 0x0000001D, } })
Unlike many of the other DSDT dumps I've looked at, there is only one interrupt listed. Full ACPI DSDT table is at [1].
Since there is no IRQ at index 5, platform_get_irq will return -ENXIO. Couldn't we fall back to index 0 in this case? I would say that if the seemingly "correct" IRQ at index 5 does not even exist, we still have a better chance of picking the right one if we try the one at index 0. Or we could check the number of interrupts that are actually available.
If I'm not mistaken then you already mentioned in another thread (the "tusb1210 probe of dwc3.0.auto.ulpi fails with EBUSY on 4.19+") thread that the DSTD of this Andriod only device has several bugs in there such as wrong GPIOs in some places, etc. and you need to do a DSDT override anyways to get some things to work, right ?
In that case I believe it would be best to just also patch up this part of the DSDT in your override and leave the current kernel code as is.
Regards,
Hans
On Sun, 16 Dec 2018 20:07:30 +0100 Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 16-12-18 19:54, Stephan Gerhold wrote:
[...]
Unlike many of the other DSDT dumps I've looked at, there is only one interrupt listed. Full ACPI DSDT table is at [1].
Since there is no IRQ at index 5, platform_get_irq will return -ENXIO. Couldn't we fall back to index 0 in this case? I would say that if the seemingly "correct" IRQ at index 5 does not even exist, we still have a better chance of picking the right one if we try the one at index 0. Or we could check the number of interrupts that are actually available.
If I'm not mistaken then you already mentioned in another thread (the "tusb1210 probe of dwc3.0.auto.ulpi fails with EBUSY on 4.19+") thread that the DSTD of this Andriod only device has several bugs in there such as wrong GPIOs in some places, etc. and you need to do a DSDT override anyways to get some things to work, right ?
In that case I believe it would be best to just also patch up this part of the DSDT in your override and leave the current kernel code as is.
FWIW that is what I did when playing with a Teclast X98 Air 3G some years ago, see: https://git.ao2.it/Teclast-X98-Air-3G_C6J6_custom_DSDT.git/
In my case I just had to fix the ordering: https://git.ao2.it/Teclast-X98-Air-3G_C6J6_custom_DSDT.git/commitdiff/f718ea...
It's been a while tho, I just committed and pushed the change now because I had forgotten back in the day.
The Makefile in the repository contains some pointers about how to build a DSDT override, what is missing is that you still have to manually append the original initrd to the file created by the Makefile.
Ciao, Antonio
Hi,
On 16-12-18 23:03, Antonio Ospite wrote:
On Sun, 16 Dec 2018 20:07:30 +0100 Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 16-12-18 19:54, Stephan Gerhold wrote:
[...]
Unlike many of the other DSDT dumps I've looked at, there is only one interrupt listed. Full ACPI DSDT table is at [1].
Since there is no IRQ at index 5, platform_get_irq will return -ENXIO. Couldn't we fall back to index 0 in this case? I would say that if the seemingly "correct" IRQ at index 5 does not even exist, we still have a better chance of picking the right one if we try the one at index 0. Or we could check the number of interrupts that are actually available.
If I'm not mistaken then you already mentioned in another thread (the "tusb1210 probe of dwc3.0.auto.ulpi fails with EBUSY on 4.19+") thread that the DSTD of this Andriod only device has several bugs in there such as wrong GPIOs in some places, etc. and you need to do a DSDT override anyways to get some things to work, right ?
In that case I believe it would be best to just also patch up this part of the DSDT in your override and leave the current kernel code as is.
FWIW that is what I did when playing with a Teclast X98 Air 3G some years ago, see: https://git.ao2.it/Teclast-X98-Air-3G_C6J6_custom_DSDT.git/
In my case I just had to fix the ordering: https://git.ao2.it/Teclast-X98-Air-3G_C6J6_custom_DSDT.git/commitdiff/f718ea...
It's been a while tho, I just committed and pushed the change now because I had forgotten back in the day.
The Makefile in the repository contains some pointers about how to build a DSDT override, what is missing is that you still have to manually append the original initrd to the file created by the Makefile.
If that is the only issue the DSTD of the Teclast-X98-Air-3G has, then this seems like a case where a DMI quirk might be a good solution, so that it will work out of the box for users trying to put Linux on there.
Regards,
Hans
On Mon, 17 Dec 2018 08:53:59 +0100 Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 16-12-18 23:03, Antonio Ospite wrote:
On Sun, 16 Dec 2018 20:07:30 +0100 Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 16-12-18 19:54, Stephan Gerhold wrote:
[...]
Unlike many of the other DSDT dumps I've looked at, there is only one interrupt listed. Full ACPI DSDT table is at [1].
Since there is no IRQ at index 5, platform_get_irq will return -ENXIO. Couldn't we fall back to index 0 in this case? I would say that if the seemingly "correct" IRQ at index 5 does not even exist, we still have a better chance of picking the right one if we try the one at index 0. Or we could check the number of interrupts that are actually available.
If I'm not mistaken then you already mentioned in another thread (the "tusb1210 probe of dwc3.0.auto.ulpi fails with EBUSY on 4.19+") thread that the DSTD of this Andriod only device has several bugs in there such as wrong GPIOs in some places, etc. and you need to do a DSDT override anyways to get some things to work, right ?
In that case I believe it would be best to just also patch up this part of the DSDT in your override and leave the current kernel code as is.
FWIW that is what I did when playing with a Teclast X98 Air 3G some years ago, see: https://git.ao2.it/Teclast-X98-Air-3G_C6J6_custom_DSDT.git/
In my case I just had to fix the ordering: https://git.ao2.it/Teclast-X98-Air-3G_C6J6_custom_DSDT.git/commitdiff/f718ea...
It's been a while tho, I just committed and pushed the change now because I had forgotten back in the day.
The Makefile in the repository contains some pointers about how to build a DSDT override, what is missing is that you still have to manually append the original initrd to the file created by the Makefile.
If that is the only issue the DSTD of the Teclast-X98-Air-3G has, then this seems like a case where a DMI quirk might be a good solution, so that it will work out of the box for users trying to put Linux on there.
I'll have to double check about that, it's been a long time since I tried booting a mainline kernel on the device.
Thanks for the suggestion, Antonio
On Sun, Dec 16, 2018 at 08:07:30PM +0100, Hans de Goede wrote:
Hi,
On 16-12-18 19:54, Stephan Gerhold wrote:
Hi,
I have an Intel Bay Trail (BYT-T) tablet that was originally shipped with Android. With the right quirks, bytcr-rt5640 is working fine, but there is a problem in sst_acpi.c that is preventing it from working with a mainline kernel:
Even though this is a BYT-T device, there is no IRQ at index 5 in the ACPI DSDT table. This means that SST will fail to probe, and actually leads to a NULL pointer dereference later when the ALSA device is first opened. (I have submitted a possible solution for this as "[PATCH] ASoC: Intel: sst: Delay machine device creation until after initialization")
The correct IRQ is actually located on index 0, just like it is already being used for BYT-CR devices. So if I force is_byt_cr() to return TRUE, everything works as expected.
Here is the relevant part from the ACPI DSDT table:
Name (_ADR, Zero) // _ADR: Address Name (_HID, "80860F28" /* Intel SST Audio DSP */) // _HID: Hardware ID Name (_CID, "80860F28" /* Intel SST Audio DSP */) // _CID: Compatible ID Name (_DDN, "Intel(R) Low Power Audio Controller - 80860F28") // _DDN: DOS Device Name Name (_SUB, "80867270") // _SUB: Subsystem ID Name (_UID, One) // _UID: Unique ID
Name (RBUF, ResourceTemplate () { Memory32Fixed (ReadWrite, 0x12345678, // Address Base 0x00200000, // Address Length _Y08) Memory32Fixed (ReadWrite, 0xFE830000, // Address Base 0x00001000, // Address Length _Y09) Memory32Fixed (ReadWrite, 0x55AA55AA, // Address Base 0x00200000, // Address Length _Y0A) Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, ) { 0x0000001D, } })
Unlike many of the other DSDT dumps I've looked at, there is only one interrupt listed. Full ACPI DSDT table is at [1].
Since there is no IRQ at index 5, platform_get_irq will return -ENXIO. Couldn't we fall back to index 0 in this case? I would say that if the seemingly "correct" IRQ at index 5 does not even exist, we still have a better chance of picking the right one if we try the one at index 0. Or we could check the number of interrupts that are actually available.
If I'm not mistaken then you already mentioned in another thread (the "tusb1210 probe of dwc3.0.auto.ulpi fails with EBUSY on 4.19+") thread that the DSTD of this Andriod only device has several bugs in there such as wrong GPIOs in some places, etc. and you need to do a DSDT override anyways to get some things to work, right ?
Yes, at the moment I use DSDT override to fix some things and add a few missing GPIOs. For example I have not yet found a nice way to fixup the ACPI parent of the Bluetooth device (it's listed under the wrong UART controller).
Note that this is rather painful, especially because I can't share the fixed table as-is with others (there is a memory address in there that is different depending on the BIOS state or something). So I'm still trying to keep the necessary changes as minimal as possible, and have the majority of things working without it. Eventually, I will find nicer workarounds and can stop doing this entirely in the future.
In that case I believe it would be best to just also patch up this part of the DSDT in your override and leave the current kernel code as is.
I have just looked through a few other DSDTs of Android based Bay Trail devices and have found this pattern in at least two more of them. So in case someone tries to run mainline on those devices in the future, I believe it would be better to fix it on the kernel side in this case. (Getting sound working has been a rather painful process for me, and took a lot of time. I would like to make this easier for others...)
I was thinking of something along the lines of:
if (platform_irq_count(pdev) == 1) byt_rvp_platform_data.res_info = &bytcr_res_info;
Pretty simple, and since all currently supported devices are either BYT-CR or have at least 5 IRQs listed, the change would not affect them in any way.
Side note: I have considered fixing this in the DSDT table a few times before but have never tried it because it kind of feels wrong to me. It would probably work, but I believe the kernel is at fault here:
I've been fixing mistakes and adding missing GPIOs to the DSDT, but is this really the case here? Everything that is needed for the driver exists in the ACPI table. If this was a BYT-CR device, it would work as-is, with no additional modifications.
Now, in order to fulfill the current expectations for BYT-T devices, I would need to add 4 additional IRQs that would never be used. But which IRQs would I list there? Dummy/invalid IRQs? To me, that does not sound like the DSDT will become any more valid.
Let me know what you think! :)
Regards,
Hans
On Mon, Dec 17, 2018 at 07:03:31PM +0100, Stephan Gerhold wrote:
Side note: I have considered fixing this in the DSDT table a few times before but have never tried it because it kind of feels wrong to me. It would probably work, but I believe the kernel is at fault here:
I've been fixing mistakes and adding missing GPIOs to the DSDT, but is this really the case here? Everything that is needed for the driver exists in the ACPI table. If this was a BYT-CR device, it would work as-is, with no additional modifications.
Now, in order to fulfill the current expectations for BYT-T devices, I would need to add 4 additional IRQs that would never be used. But which IRQs would I list there? Dummy/invalid IRQs? To me, that does not sound like the DSDT will become any more valid.
Let me know what you think! :)
This thread appears to have died without a conclusion I can see? I don't have strong feelings here, it seems like it's a choice between different evils so people working on the platform should make the call.
On 1/9/19 1:22 PM, Mark Brown wrote:
On Mon, Dec 17, 2018 at 07:03:31PM +0100, Stephan Gerhold wrote:
Side note: I have considered fixing this in the DSDT table a few times before but have never tried it because it kind of feels wrong to me. It would probably work, but I believe the kernel is at fault here: I've been fixing mistakes and adding missing GPIOs to the DSDT, but is this really the case here? Everything that is needed for the driver exists in the ACPI table. If this was a BYT-CR device, it would work as-is, with no additional modifications. Now, in order to fulfill the current expectations for BYT-T devices, I would need to add 4 additional IRQs that would never be used. But which IRQs would I list there? Dummy/invalid IRQs? To me, that does not sound like the DSDT will become any more valid. Let me know what you think! :)
This thread appears to have died without a conclusion I can see? I don't have strong feelings here, it seems like it's a choice between different evils so people working on the platform should make the call.
this was fixed by the v2 patch applied on January 4?
[alsa-devel] Applied "ASoC: Intel: sst: Fallback to BYT-CR if IRQ 5 is missing" to the asoc tree
On Wed, Jan 09, 2019 at 03:10:53PM -0600, Pierre-Louis Bossart wrote:
On 1/9/19 1:22 PM, Mark Brown wrote:
This thread appears to have died without a conclusion I can see? I don't have strong feelings here, it seems like it's a choice between different evils so people working on the platform should make the call.
this was fixed by the v2 patch applied on January 4?
[alsa-devel] Applied "ASoC: Intel: sst: Fallback to BYT-CR if IRQ 5 is missing" to the asoc tree
That's entirely possible, the subject line did change so I didn't immediately notice it was the same quirk.
On 12/16/18 12:54 PM, Stephan Gerhold wrote:
Hi,
I have an Intel Bay Trail (BYT-T) tablet that was originally shipped with Android. With the right quirks, bytcr-rt5640 is working fine, but there is a problem in sst_acpi.c that is preventing it from working with a mainline kernel:
Even though this is a BYT-T device, there is no IRQ at index 5 in the ACPI DSDT table. This means that SST will fail to probe, and actually leads to a NULL pointer dereference later when the ALSA device is first opened. (I have submitted a possible solution for this as "[PATCH] ASoC: Intel: sst: Delay machine device creation until after initialization")
The correct IRQ is actually located on index 0, just like it is already being used for BYT-CR devices. So if I force is_byt_cr() to return TRUE, everything works as expected.
So the root cause of your problem is that the detection of byt-cr doesn't work? That would be a first.
Can you please double-check that CONFIG_IOSF_MBI is enabled and provide a trace of the bios status in this piece of code:
/* bits 26:27 mirror PMIC options */ bios_status = (bios_status >> 26) & 3;
if ((bios_status == 1) || (bios_status == 3)) *bytcr = true; else dev_info(dev, "BYT-CR not detected\n");
Here is the relevant part from the ACPI DSDT table:
Name (_ADR, Zero) // _ADR: Address Name (_HID, "80860F28" /* Intel SST Audio DSP */) // _HID: Hardware ID Name (_CID, "80860F28" /* Intel SST Audio DSP */) // _CID: Compatible ID Name (_DDN, "Intel(R) Low Power Audio Controller - 80860F28") // _DDN: DOS Device Name Name (_SUB, "80867270") // _SUB: Subsystem ID Name (_UID, One) // _UID: Unique ID
Name (RBUF, ResourceTemplate () { Memory32Fixed (ReadWrite, 0x12345678, // Address Base 0x00200000, // Address Length _Y08) Memory32Fixed (ReadWrite, 0xFE830000, // Address Base 0x00001000, // Address Length _Y09) Memory32Fixed (ReadWrite, 0x55AA55AA, // Address Base 0x00200000, // Address Length _Y0A) Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, ) { 0x0000001D, } })
Unlike many of the other DSDT dumps I've looked at, there is only one interrupt listed. Full ACPI DSDT table is at [1].
Since there is no IRQ at index 5, platform_get_irq will return -ENXIO. Couldn't we fall back to index 0 in this case? I would say that if the seemingly "correct" IRQ at index 5 does not even exist, we still have a better chance of picking the right one if we try the one at index 0. Or we could check the number of interrupts that are actually available.
The other option would be some kind of DMI-based quirk, but personally I would prefer to avoid that.. (In my opinion, there is way too much device specific code with the quirks etc already...)
Or do you have any other suggestions?
Thanks, Stephan
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Mon, Dec 17, 2018 at 08:52:46AM -0600, Pierre-Louis Bossart wrote:
On 12/16/18 12:54 PM, Stephan Gerhold wrote:
Hi,
I have an Intel Bay Trail (BYT-T) tablet that was originally shipped with Android. With the right quirks, bytcr-rt5640 is working fine, but there is a problem in sst_acpi.c that is preventing it from working with a mainline kernel:
Even though this is a BYT-T device, there is no IRQ at index 5 in the ACPI DSDT table. This means that SST will fail to probe, and actually leads to a NULL pointer dereference later when the ALSA device is first opened. (I have submitted a possible solution for this as "[PATCH] ASoC: Intel: sst: Delay machine device creation until after initialization")
The correct IRQ is actually located on index 0, just like it is already being used for BYT-CR devices. So if I force is_byt_cr() to return TRUE, everything works as expected.
So the root cause of your problem is that the detection of byt-cr doesn't work? That would be a first.
No. is_byt_cr() works correctly, as my device is a BYT-T (not a BYT-CR) device. :)
The problem here is that the kernel expects the IRQ at index 5 for BYT-T devices, but my device has only a single IRQ listed. Forcing is_byt_cr() to return TRUE is just a workaround to make it use the IRQ at index 0 (which is the correct one).
Currently, sst_acpi supports these two variations: - BYT-T: 5 IRQs listed -> acpi_ipc_irq_index = 5 - BYT-CR: 5 IRQs listed -> acpi_ipc_irq_index = 0
On my device (and a few other Android based BYT-T devices) I have found: - BYT-T: 1 IRQ listed -> acpi_ipc_irq_index = 0 but at the moment the kernel attempts to use acpi_ipc_irq_index = 5 from BYT-T above.
Can you please double-check that CONFIG_IOSF_MBI is enabled and provide a trace of the bios status in this piece of code:
/* bits 26:27 mirror PMIC options */ bios_status = (bios_status >> 26) & 3;
if ((bios_status == 1) || (bios_status == 3)) *bytcr = true; else dev_info(dev, "BYT-CR not detected\n");
Here is the relevant part from the ACPI DSDT table:
Name (_ADR, Zero) // _ADR: Address Name (_HID, "80860F28" /* Intel SST Audio DSP */) // _HID: Hardware ID Name (_CID, "80860F28" /* Intel SST Audio DSP */) // _CID: Compatible ID Name (_DDN, "Intel(R) Low Power Audio Controller - 80860F28") // _DDN: DOS Device Name Name (_SUB, "80867270") // _SUB: Subsystem ID Name (_UID, One) // _UID: Unique ID
Name (RBUF, ResourceTemplate () { Memory32Fixed (ReadWrite, 0x12345678, // Address Base 0x00200000, // Address Length _Y08) Memory32Fixed (ReadWrite, 0xFE830000, // Address Base 0x00001000, // Address Length _Y09) Memory32Fixed (ReadWrite, 0x55AA55AA, // Address Base 0x00200000, // Address Length _Y0A) Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, ) { 0x0000001D, } })
Unlike many of the other DSDT dumps I've looked at, there is only one interrupt listed. Full ACPI DSDT table is at [1].
Since there is no IRQ at index 5, platform_get_irq will return -ENXIO. Couldn't we fall back to index 0 in this case? I would say that if the seemingly "correct" IRQ at index 5 does not even exist, we still have a better chance of picking the right one if we try the one at index 0. Or we could check the number of interrupts that are actually available.
The other option would be some kind of DMI-based quirk, but personally I would prefer to avoid that.. (In my opinion, there is way too much device specific code with the quirks etc already...)
Or do you have any other suggestions?
Thanks, Stephan
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 12/17/18 12:17 PM, Stephan Gerhold wrote:
On Mon, Dec 17, 2018 at 08:52:46AM -0600, Pierre-Louis Bossart wrote:
On 12/16/18 12:54 PM, Stephan Gerhold wrote:
Hi,
I have an Intel Bay Trail (BYT-T) tablet that was originally shipped with Android. With the right quirks, bytcr-rt5640 is working fine, but there is a problem in sst_acpi.c that is preventing it from working with a mainline kernel:
Even though this is a BYT-T device, there is no IRQ at index 5 in the ACPI DSDT table. This means that SST will fail to probe, and actually leads to a NULL pointer dereference later when the ALSA device is first opened. (I have submitted a possible solution for this as "[PATCH] ASoC: Intel: sst: Delay machine device creation until after initialization")
The correct IRQ is actually located on index 0, just like it is already being used for BYT-CR devices. So if I force is_byt_cr() to return TRUE, everything works as expected.
So the root cause of your problem is that the detection of byt-cr doesn't work? That would be a first.
No. is_byt_cr() works correctly, as my device is a BYT-T (not a BYT-CR) device. :)
What information is your analysis based on and how do you infer this conclusion? The BYT-T and BYT-CR silicon dies are identical, product documentation can barely be trusted and it's a package difference that can only be tested indirectly.
I don't mean to dismiss your claim, just want to find out if this is a case where the PMIC-type-based byt_cr detection fails or if we have a BIOS issue. Another smoking gun is if you find in your code traces of SSP0 being used.
The problem here is that the kernel expects the IRQ at index 5 for BYT-T devices, but my device has only a single IRQ listed. Forcing is_byt_cr() to return TRUE is just a workaround to make it use the IRQ at index 0 (which is the correct one).
Currently, sst_acpi supports these two variations:
- BYT-T: 5 IRQs listed -> acpi_ipc_irq_index = 5
- BYT-CR: 5 IRQs listed -> acpi_ipc_irq_index = 0
On my device (and a few other Android based BYT-T devices) I have found:
- BYT-T: 1 IRQ listed -> acpi_ipc_irq_index = 0
but at the moment the kernel attempts to use acpi_ipc_irq_index = 5 from BYT-T above.
Can you please double-check that CONFIG_IOSF_MBI is enabled and provide a trace of the bios status in this piece of code:
/* bits 26:27 mirror PMIC options */ bios_status = (bios_status >> 26) & 3;
if ((bios_status == 1) || (bios_status == 3)) *bytcr = true; else dev_info(dev, "BYT-CR not detected\n");
Here is the relevant part from the ACPI DSDT table:
Name (_ADR, Zero) // _ADR: Address Name (_HID, "80860F28" /* Intel SST Audio DSP */) // _HID: Hardware ID Name (_CID, "80860F28" /* Intel SST Audio DSP */) // _CID: Compatible ID Name (_DDN, "Intel(R) Low Power Audio Controller - 80860F28") // _DDN: DOS Device Name Name (_SUB, "80867270") // _SUB: Subsystem ID Name (_UID, One) // _UID: Unique ID Name (RBUF, ResourceTemplate () { Memory32Fixed (ReadWrite, 0x12345678, // Address Base 0x00200000, // Address Length _Y08) Memory32Fixed (ReadWrite, 0xFE830000, // Address Base 0x00001000, // Address Length _Y09) Memory32Fixed (ReadWrite, 0x55AA55AA, // Address Base 0x00200000, // Address Length _Y0A) Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, ) { 0x0000001D, } })Unlike many of the other DSDT dumps I've looked at, there is only one interrupt listed. Full ACPI DSDT table is at [1].
Since there is no IRQ at index 5, platform_get_irq will return -ENXIO. Couldn't we fall back to index 0 in this case? I would say that if the seemingly "correct" IRQ at index 5 does not even exist, we still have a better chance of picking the right one if we try the one at index 0. Or we could check the number of interrupts that are actually available.
The other option would be some kind of DMI-based quirk, but personally I would prefer to avoid that.. (In my opinion, there is way too much device specific code with the quirks etc already...)
Or do you have any other suggestions?
Thanks, Stephan
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Mon, Dec 17, 2018 at 12:29:43PM -0600, Pierre-Louis Bossart wrote:
On 12/17/18 12:17 PM, Stephan Gerhold wrote:
On Mon, Dec 17, 2018 at 08:52:46AM -0600, Pierre-Louis Bossart wrote:
On 12/16/18 12:54 PM, Stephan Gerhold wrote:
Hi,
I have an Intel Bay Trail (BYT-T) tablet that was originally shipped with Android. With the right quirks, bytcr-rt5640 is working fine, but there is a problem in sst_acpi.c that is preventing it from working with a mainline kernel:
Even though this is a BYT-T device, there is no IRQ at index 5 in the ACPI DSDT table. This means that SST will fail to probe, and actually leads to a NULL pointer dereference later when the ALSA device is first opened. (I have submitted a possible solution for this as "[PATCH] ASoC: Intel: sst: Delay machine device creation until after initialization")
The correct IRQ is actually located on index 0, just like it is already being used for BYT-CR devices. So if I force is_byt_cr() to return TRUE, everything works as expected.
So the root cause of your problem is that the detection of byt-cr doesn't work? That would be a first.
No. is_byt_cr() works correctly, as my device is a BYT-T (not a BYT-CR) device. :)
What information is your analysis based on and how do you infer this conclusion? The BYT-T and BYT-CR silicon dies are identical, product documentation can barely be trusted and it's a package difference that can only be tested indirectly.
Okay, sorry - I'm not _that_ sure. :) It's mostly based on something called "spid" that was being used in the stock Android kernel on the device and some code from the stock kernel I've looked at.
I don't mind checking this again to be absolutely sure :)
The call to iosf_mbi_read() returns 0x400b0100
/* bits 26:27 mirror PMIC options */ bios_status = (bios_status >> 26) & 3;
Results in bios_status = 0x0
The stock kernel printed this on every startup:
SPID updated according to ACPI Table: spid customer id : 0000 spid vendor id : 0000 spid manufacturer id : 00ff spid platform family id : 0007 --> INTEL_BYT_TABLET spid product line id : 0000 --> INTEL_BYT_TABLET_BLK_PRO spid hardware id : 0004 --> BYT_TABLET_BLK_8PR0 /* Bay Lake FFRD-8 PR0 */ spid fru[4..0] : 00 00 00 00 00 spid fru[9..5] : 00 00 00 00 00
Based on spid.h [1] I added the "-->" above. Then I guessed that this is BYT-T (there is another "BYT T CR V2" value), but to be honest I don't know for sure.
[1]: https://github.com/me176c-dev/me176c-kernel/blob/stock/kernel/arch/x86/inclu...
I don't mean to dismiss your claim, just want to find out if this is a case where the PMIC-type-based byt_cr detection fails or if we have a BIOS issue. Another smoking gun is if you find in your code traces of SSP0 being used.
The quirks to get sound working with bytcr-rt5640 on that device are: BYT_RT5640_SSP0_AIF1 | BYT_RT5640_IN1_MAP | BYT_RT5640_MCLK_EN
I guess this means that SSP0 is being used?
The problem here is that the kernel expects the IRQ at index 5 for BYT-T devices, but my device has only a single IRQ listed. Forcing is_byt_cr() to return TRUE is just a workaround to make it use the IRQ at index 0 (which is the correct one).
Currently, sst_acpi supports these two variations:
- BYT-T: 5 IRQs listed -> acpi_ipc_irq_index = 5
- BYT-CR: 5 IRQs listed -> acpi_ipc_irq_index = 0
On my device (and a few other Android based BYT-T devices) I have found:
- BYT-T: 1 IRQ listed -> acpi_ipc_irq_index = 0
but at the moment the kernel attempts to use acpi_ipc_irq_index = 5 from BYT-T above.
Can you please double-check that CONFIG_IOSF_MBI is enabled and provide a trace of the bios status in this piece of code:
/* bits 26:27 mirror PMIC options */ bios_status = (bios_status >> 26) & 3;
if ((bios_status == 1) || (bios_status == 3)) *bytcr = true; else dev_info(dev, "BYT-CR not detected\n");
Here is the relevant part from the ACPI DSDT table:
Name (_ADR, Zero) // _ADR: Address Name (_HID, "80860F28" /* Intel SST Audio DSP */) // _HID: Hardware ID Name (_CID, "80860F28" /* Intel SST Audio DSP */) // _CID: Compatible ID Name (_DDN, "Intel(R) Low Power Audio Controller - 80860F28") // _DDN: DOS Device Name Name (_SUB, "80867270") // _SUB: Subsystem ID Name (_UID, One) // _UID: Unique ID Name (RBUF, ResourceTemplate () { Memory32Fixed (ReadWrite, 0x12345678, // Address Base 0x00200000, // Address Length _Y08) Memory32Fixed (ReadWrite, 0xFE830000, // Address Base 0x00001000, // Address Length _Y09) Memory32Fixed (ReadWrite, 0x55AA55AA, // Address Base 0x00200000, // Address Length _Y0A) Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, ) { 0x0000001D, } })Unlike many of the other DSDT dumps I've looked at, there is only one interrupt listed. Full ACPI DSDT table is at [1].
Since there is no IRQ at index 5, platform_get_irq will return -ENXIO. Couldn't we fall back to index 0 in this case? I would say that if the seemingly "correct" IRQ at index 5 does not even exist, we still have a better chance of picking the right one if we try the one at index 0. Or we could check the number of interrupts that are actually available.
The other option would be some kind of DMI-based quirk, but personally I would prefer to avoid that.. (In my opinion, there is way too much device specific code with the quirks etc already...)
Or do you have any other suggestions?
Thanks, Stephan
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Thanks for the additional information.
The call to iosf_mbi_read() returns 0x400b0100
/* bits 26:27 mirror PMIC options */ bios_status = (bios_status >> 26) & 3;
Results in bios_status = 0x0
So that's a fail.
The stock kernel printed this on every startup:
SPID updated according to ACPI Table: spid customer id : 0000 spid vendor id : 0000 spid manufacturer id : 00ff spid platform family id : 0007 --> INTEL_BYT_TABLET spid product line id : 0000 --> INTEL_BYT_TABLET_BLK_PRO spid hardware id : 0004 --> BYT_TABLET_BLK_8PR0 /* Bay Lake FFRD-8 PR0 */ spid fru[4..0] : 00 00 00 00 00 spid fru[9..5] : 00 00 00 00 00
Based on spid.h [1] I added the "-->" above. Then I guessed that this is BYT-T (there is another "BYT T CR V2" value), but to be honest I don't know for sure.
Oh man, Bay Lake...this must be at least 6 years old and 30+ kernel versions behind... Only a couple of years and it'll be a collector item :-)
I can't recall any of the details so we'll have to wing it. it could be that it was baytrail-T but with the software/BIOS for Baytrail-Cr, who knows.
I don't mean to dismiss your claim, just want to find out if this is a case where the PMIC-type-based byt_cr detection fails or if we have a BIOS issue. Another smoking gun is if you find in your code traces of SSP0 being used.
The quirks to get sound working with bytcr-rt5640 on that device are: BYT_RT5640_SSP0_AIF1 | BYT_RT5640_IN1_MAP | BYT_RT5640_MCLK_EN
I guess this means that SSP0 is being used?
Yes indeed, and that makes me think we should force this device to look like Baytrail-CR.
You can do this with a DMI-based quirk (preferably in is_byt_cr directly so that I can reuse the code when I move it to a helper at some point).
Also I guess you'd need a second quirk in bytcr_rt5640 since the default is SSP0-AIF2.
-Pierre
On Mon, Dec 17, 2018 at 01:39:13PM -0600, Pierre-Louis Bossart wrote:
Thanks for the additional information.
The call to iosf_mbi_read() returns 0x400b0100
/* bits 26:27 mirror PMIC options */ bios_status = (bios_status >> 26) & 3;
Results in bios_status = 0x0
So that's a fail.
The stock kernel printed this on every startup:
SPID updated according to ACPI Table: spid customer id : 0000 spid vendor id : 0000 spid manufacturer id : 00ff spid platform family id : 0007 --> INTEL_BYT_TABLET spid product line id : 0000 --> INTEL_BYT_TABLET_BLK_PRO spid hardware id : 0004 --> BYT_TABLET_BLK_8PR0 /* Bay Lake FFRD-8 PR0 */ spid fru[4..0] : 00 00 00 00 00 spid fru[9..5] : 00 00 00 00 00
Based on spid.h [1] I added the "-->" above. Then I guessed that this is BYT-T (there is another "BYT T CR V2" value), but to be honest I don't know for sure.
Oh man, Bay Lake...this must be at least 6 years old and 30+ kernel versions behind... Only a couple of years and it'll be a collector item :-)
Yeah, the device was shipped with a 3.10 kernel but I believe that file was just copied from an earlier 3.4 kernel. I have never bothered to even try to compile that thing, I just use it as reference every now and then. :)
I can't recall any of the details so we'll have to wing it. it could be that it was baytrail-T but with the software/BIOS for Baytrail-Cr, who knows.
I don't mean to dismiss your claim, just want to find out if this is a case where the PMIC-type-based byt_cr detection fails or if we have a BIOS issue. Another smoking gun is if you find in your code traces of SSP0 being used.
The quirks to get sound working with bytcr-rt5640 on that device are: BYT_RT5640_SSP0_AIF1 | BYT_RT5640_IN1_MAP | BYT_RT5640_MCLK_EN
I guess this means that SSP0 is being used?
Yes indeed, and that makes me think we should force this device to look like Baytrail-CR.
You can do this with a DMI-based quirk (preferably in is_byt_cr directly so that I can reuse the code when I move it to a helper at some point).
Okay - thanks! One last question: I was looking at the ACPI DSDT tables of some similar devices and have found two others that look the same (only one IRQ listed). In this case, the BYT-T acpi_ipc_irq_index = 5 will never work, and we will definitely have a better chances with trying Baytrail-CR.
One of them actually had a similar patch proposed at [1] (although they did it in a weird way and also need an extra machine driver).
We could also detect this situation in a generic way with something like
if (platform_irq_count(pdev) == 1) { *bytcr = true; return 0; }
... instead of a DMI quirk. What do you think?
[1]: https://patchwork.kernel.org/patch/9764493/#20539549
Also I guess you'd need a second quirk in bytcr_rt5640 since the default is SSP0-AIF2.
-Pierre
On Mon, Dec 17, 2018 at 09:32:42PM +0100, Stephan Gerhold wrote:
On Mon, Dec 17, 2018 at 01:39:13PM -0600, Pierre-Louis Bossart wrote:
Thanks for the additional information.
The call to iosf_mbi_read() returns 0x400b0100
/* bits 26:27 mirror PMIC options */ bios_status = (bios_status >> 26) & 3;
Results in bios_status = 0x0
So that's a fail.
The stock kernel printed this on every startup:
SPID updated according to ACPI Table: spid customer id : 0000 spid vendor id : 0000 spid manufacturer id : 00ff spid platform family id : 0007 --> INTEL_BYT_TABLET spid product line id : 0000 --> INTEL_BYT_TABLET_BLK_PRO spid hardware id : 0004 --> BYT_TABLET_BLK_8PR0 /* Bay Lake FFRD-8 PR0 */ spid fru[4..0] : 00 00 00 00 00 spid fru[9..5] : 00 00 00 00 00
Based on spid.h [1] I added the "-->" above. Then I guessed that this is BYT-T (there is another "BYT T CR V2" value), but to be honest I don't know for sure.
Oh man, Bay Lake...this must be at least 6 years old and 30+ kernel versions behind... Only a couple of years and it'll be a collector item :-)
Yeah, the device was shipped with a 3.10 kernel but I believe that file was just copied from an earlier 3.4 kernel. I have never bothered to even try to compile that thing, I just use it as reference every now and then. :)
I can't recall any of the details so we'll have to wing it. it could be that it was baytrail-T but with the software/BIOS for Baytrail-Cr, who knows.
I don't mean to dismiss your claim, just want to find out if this is a case where the PMIC-type-based byt_cr detection fails or if we have a BIOS issue. Another smoking gun is if you find in your code traces of SSP0 being used.
The quirks to get sound working with bytcr-rt5640 on that device are: BYT_RT5640_SSP0_AIF1 | BYT_RT5640_IN1_MAP | BYT_RT5640_MCLK_EN
I guess this means that SSP0 is being used?
Yes indeed, and that makes me think we should force this device to look like Baytrail-CR.
You can do this with a DMI-based quirk (preferably in is_byt_cr directly so that I can reuse the code when I move it to a helper at some point).
Okay - thanks! One last question: I was looking at the ACPI DSDT tables of some similar devices and have found two others that look the same (only one IRQ listed). In this case, the BYT-T acpi_ipc_irq_index = 5 will never work, and we will definitely have a better chances with trying Baytrail-CR.
One of them actually had a similar patch proposed at [1] (although they did it in a weird way and also need an extra machine driver).
We could also detect this situation in a generic way with something like
if (platform_irq_count(pdev) == 1) { *bytcr = true; return 0; }
... instead of a DMI quirk. What do you think?
To avoid confusion: The existing PMIC-type based is_byt_cr() detection would be used in all other cases (i.e. if irq_count != 1), so it won't make any difference for the devices that are already working fine. (Most BYT-CR devices seem to have 5 IRQs listed)
So it's more like
if (platform_irq_count(pdev) == 1) { *bytcr = true; } else { // pmic-type based detection... }
with platform_irq_count == 1 as condition for the "quirk".
Also I guess you'd need a second quirk in bytcr_rt5640 since the default is SSP0-AIF2.
-Pierre
The quirks to get sound working with bytcr-rt5640 on that device are: BYT_RT5640_SSP0_AIF1 | BYT_RT5640_IN1_MAP | BYT_RT5640_MCLK_EN
I guess this means that SSP0 is being used?
Yes indeed, and that makes me think we should force this device to look like Baytrail-CR.
You can do this with a DMI-based quirk (preferably in is_byt_cr directly so that I can reuse the code when I move it to a helper at some point).
Okay - thanks! One last question: I was looking at the ACPI DSDT tables of some similar devices and have found two others that look the same (only one IRQ listed). In this case, the BYT-T acpi_ipc_irq_index = 5 will never work, and we will definitely have a better chances with trying Baytrail-CR.
One of them actually had a similar patch proposed at [1] (although they did it in a weird way and also need an extra machine driver).
We could also detect this situation in a generic way with something like
if (platform_irq_count(pdev) == 1) { *bytcr = true; return 0; }
... instead of a DMI quirk. What do you think?
To avoid confusion: The existing PMIC-type based is_byt_cr() detection would be used in all other cases (i.e. if irq_count != 1), so it won't make any difference for the devices that are already working fine. (Most BYT-CR devices seem to have 5 IRQs listed)
So it's more like
if (platform_irq_count(pdev) == 1) { *bytcr = true; } else { // pmic-type based detection... }
with platform_irq_count == 1 as condition for the "quirk".
The solution seems appealing but
1) does it really work? I am not sure an index=5 means there are 5 interrupts.
2) the test would affect all existing devices, and there's so much hardware proliferation that proving this change in harmless might be difficult. I personally only have one BYT-T (ASus T100) device left and it's not very reliable. Hans seems to have a ton of devices but they are mostly Byt-Cr?
On Mon, Dec 17, 2018 at 08:13:36PM -0600, Pierre-Louis Bossart wrote:
The quirks to get sound working with bytcr-rt5640 on that device are: BYT_RT5640_SSP0_AIF1 | BYT_RT5640_IN1_MAP | BYT_RT5640_MCLK_EN
I guess this means that SSP0 is being used?
Yes indeed, and that makes me think we should force this device to look like Baytrail-CR.
You can do this with a DMI-based quirk (preferably in is_byt_cr directly so that I can reuse the code when I move it to a helper at some point).
Okay - thanks! One last question: I was looking at the ACPI DSDT tables of some similar devices and have found two others that look the same (only one IRQ listed). In this case, the BYT-T acpi_ipc_irq_index = 5 will never work, and we will definitely have a better chances with trying Baytrail-CR.
One of them actually had a similar patch proposed at [1] (although they did it in a weird way and also need an extra machine driver).
We could also detect this situation in a generic way with something like
if (platform_irq_count(pdev) == 1) { *bytcr = true; return 0; }
... instead of a DMI quirk. What do you think?
To avoid confusion: The existing PMIC-type based is_byt_cr() detection would be used in all other cases (i.e. if irq_count != 1), so it won't make any difference for the devices that are already working fine. (Most BYT-CR devices seem to have 5 IRQs listed)
So it's more like
if (platform_irq_count(pdev) == 1) { *bytcr = true; } else { // pmic-type based detection... }
with platform_irq_count == 1 as condition for the "quirk".
The solution seems appealing but
- does it really work? I am not sure an index=5 means there are 5
interrupts.
Yes, I believe that it means that there need to be (at least) 5 interrupts.
I have checked the source code of platform_get_irq. When you do platform_get_irq(pdev, /* index = */ 5) (as done for BYT-T) it first calls
platform_get_resource(/* type = */ IORESOURCE_IRQ, /* num = */ 5)
to lookup the resource. That method is really simple and looks like
for (i = 0; i < dev->num_resources; i++) { struct resource *r = &dev->resource[i];
if (type == resource_type(r) && num-- == 0) return r; }
So when you request an IRQ at index=5, it loops through all resources, skips the first 5 IRQs and returns the 6th one (on my device, it returns NULL because there are not enough IRQs listed).
There is a bit more magic in platform_irq_count (it looks up all IRQs and does not count invalid ones), so to be absolutely safe we could just check platform_get_resource(IRQ, 5) == NULL early. If it returns NULL, then platform_get_irq(pdev, 5) will also return -ENXIO, so treating the device as BYT-T is guaranteed to fail. In this case, we have better chances when we assume BYT-CR.
Example patch: (I have added it in probe instead of is_byt_cr() because it requires the platform device, plus I think it might be better to differentiate the messages in the kernel log..)
--- a/sound/soc/intel/atom/sst/sst_acpi.c +++ b/sound/soc/intel/atom/sst/sst_acpi.c @@ -337,6 +337,16 @@ static int sst_acpi_probe(struct platform_device *pdev) if (!((ret < 0) || (bytcr == false))) { dev_info(dev, "Detected Baytrail-CR platform\n");
/* override resource info */ byt_rvp_platform_data.res_info = &bytcr_res_info; + } else if (platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) { + /* + * Some devices detected as BYT-T have only a single IRQ listed. + * In this case, platform_get_irq with index 5 will return -ENXIO. + * Fall back to the BYT-CR resource info to use the correct IRQ. + */ + dev_info(dev, "Falling back to Baytrail-CR platform\n"); + + /* override resource info */ + byt_rvp_platform_data.res_info = &bytcr_res_info; }
- the test would affect all existing devices, and there's so much hardware
proliferation that proving this change in harmless might be difficult. I personally only have one BYT-T (ASus T100) device left and it's not very reliable. Hans seems to have a ton of devices but they are mostly Byt-Cr?
With the updated patch above I believe there is literally no way this can break sound on any device. The condition only evaluates to true if SST would normally fail to probe later anyway.
I have tested the patch above on my device with: - as-is, without any modifications: -> "Falling back to Baytrail-CR platform", sound now working - a simulated "BYT-T" device: (copied the IRQs from the DSDT of the T100TA) -> "BYT-CR not detected" - uses 5th IRQ, sound working - a simulated "BYT-CR" device (made is_byt_cr() return "true" and copied the IRQs from the DSDT of the T100TAF) -> "Detected Baytrail-CR platform" - uses IRQ at index 0, sound working
Let me know what you think!
On 12/19/18 7:07 AM, Stephan Gerhold wrote:
On Mon, Dec 17, 2018 at 08:13:36PM -0600, Pierre-Louis Bossart wrote:
The quirks to get sound working with bytcr-rt5640 on that device are: BYT_RT5640_SSP0_AIF1 | BYT_RT5640_IN1_MAP | BYT_RT5640_MCLK_EN
I guess this means that SSP0 is being used?
Yes indeed, and that makes me think we should force this device to look like Baytrail-CR.
You can do this with a DMI-based quirk (preferably in is_byt_cr directly so that I can reuse the code when I move it to a helper at some point).
Okay - thanks! One last question: I was looking at the ACPI DSDT tables of some similar devices and have found two others that look the same (only one IRQ listed). In this case, the BYT-T acpi_ipc_irq_index = 5 will never work, and we will definitely have a better chances with trying Baytrail-CR.
One of them actually had a similar patch proposed at [1] (although they did it in a weird way and also need an extra machine driver).
We could also detect this situation in a generic way with something like
if (platform_irq_count(pdev) == 1) { *bytcr = true;return 0; }
... instead of a DMI quirk. What do you think?
To avoid confusion: The existing PMIC-type based is_byt_cr() detection would be used in all other cases (i.e. if irq_count != 1), so it won't make any difference for the devices that are already working fine. (Most BYT-CR devices seem to have 5 IRQs listed)
So it's more like
if (platform_irq_count(pdev) == 1) { *bytcr = true; } else { // pmic-type based detection... }with platform_irq_count == 1 as condition for the "quirk".
The solution seems appealing but
- does it really work? I am not sure an index=5 means there are 5
interrupts.
Yes, I believe that it means that there need to be (at least) 5 interrupts.
I have checked the source code of platform_get_irq. When you do platform_get_irq(pdev, /* index = */ 5) (as done for BYT-T) it first calls
platform_get_resource(/* type = */ IORESOURCE_IRQ, /* num = */ 5)to lookup the resource. That method is really simple and looks like
for (i = 0; i < dev->num_resources; i++) { struct resource *r = &dev->resource[i];
if (type == resource_type(r) && num-- == 0) return r;}
So when you request an IRQ at index=5, it loops through all resources, skips the first 5 IRQs and returns the 6th one (on my device, it returns NULL because there are not enough IRQs listed).
There is a bit more magic in platform_irq_count (it looks up all IRQs and does not count invalid ones), so to be absolutely safe we could just check platform_get_resource(IRQ, 5) == NULL early. If it returns NULL, then platform_get_irq(pdev, 5) will also return -ENXIO, so treating the device as BYT-T is guaranteed to fail. In this case, we have better chances when we assume BYT-CR.
Example patch: (I have added it in probe instead of is_byt_cr() because it requires the platform device, plus I think it might be better to differentiate the messages in the kernel log..)
--- a/sound/soc/intel/atom/sst/sst_acpi.c +++ b/sound/soc/intel/atom/sst/sst_acpi.c @@ -337,6 +337,16 @@ static int sst_acpi_probe(struct platform_device *pdev) if (!((ret < 0) || (bytcr == false))) { dev_info(dev, "Detected Baytrail-CR platform\n");
/* override resource info */ byt_rvp_platform_data.res_info = &bytcr_res_info;
- } else if (platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) {
/** Some devices detected as BYT-T have only a single IRQ listed.* In this case, platform_get_irq with index 5 will return -ENXIO.* Fall back to the BYT-CR resource info to use the correct IRQ.*/dev_info(dev, "Falling back to Baytrail-CR platform\n");/* override resource info */byt_rvp_platform_data.res_info = &bytcr_res_info;
- the test would affect all existing devices, and there's so much hardware
proliferation that proving this change in harmless might be difficult. I personally only have one BYT-T (ASus T100) device left and it's not very reliable. Hans seems to have a ton of devices but they are mostly Byt-Cr?
With the updated patch above I believe there is literally no way this can break sound on any device. The condition only evaluates to true if SST would normally fail to probe later anyway.
I have tested the patch above on my device with:
- as-is, without any modifications: -> "Falling back to Baytrail-CR platform", sound now working
- a simulated "BYT-T" device: (copied the IRQs from the DSDT of the T100TA) -> "BYT-CR not detected" - uses 5th IRQ, sound working
- a simulated "BYT-CR" device (made is_byt_cr() return "true" and copied the IRQs from the DSDT of the T100TAF) -> "Detected Baytrail-CR platform" - uses IRQ at index 0, sound working
Let me know what you think!
Sounds good, playing with resources is what I had in mind rather than an interrupt count which isn't necessarily safe. The only improvement I would suggest is to add this test inside of is_byt_cr(). This routine will be moved as a helper outside of sst_acpi to be reused for SOF, so if we can make this test more self-contained it's more future-proof.
Hi,
On 19-12-18 15:04, Pierre-Louis Bossart wrote:
On 12/19/18 7:07 AM, Stephan Gerhold wrote:
On Mon, Dec 17, 2018 at 08:13:36PM -0600, Pierre-Louis Bossart wrote:
> The quirks to get sound working with bytcr-rt5640 on that device are: > BYT_RT5640_SSP0_AIF1 | BYT_RT5640_IN1_MAP | BYT_RT5640_MCLK_EN > > I guess this means that SSP0 is being used? Yes indeed, and that makes me think we should force this device to look like Baytrail-CR.
You can do this with a DMI-based quirk (preferably in is_byt_cr directly so that I can reuse the code when I move it to a helper at some point).
Okay - thanks! One last question: I was looking at the ACPI DSDT tables of some similar devices and have found two others that look the same (only one IRQ listed). In this case, the BYT-T acpi_ipc_irq_index = 5 will never work, and we will definitely have a better chances with trying Baytrail-CR.
One of them actually had a similar patch proposed at [1] (although they did it in a weird way and also need an extra machine driver).
We could also detect this situation in a generic way with something like
if (platform_irq_count(pdev) == 1) { *bytcr = true; return 0; }
... instead of a DMI quirk. What do you think?
To avoid confusion: The existing PMIC-type based is_byt_cr() detection would be used in all other cases (i.e. if irq_count != 1), so it won't make any difference for the devices that are already working fine. (Most BYT-CR devices seem to have 5 IRQs listed)
So it's more like
if (platform_irq_count(pdev) == 1) { *bytcr = true; } else { // pmic-type based detection... }
with platform_irq_count == 1 as condition for the "quirk".
The solution seems appealing but
- does it really work? I am not sure an index=5 means there are 5
interrupts.
Yes, I believe that it means that there need to be (at least) 5 interrupts.
I have checked the source code of platform_get_irq. When you do platform_get_irq(pdev, /* index = */ 5) (as done for BYT-T) it first calls
platform_get_resource(/* type = */ IORESOURCE_IRQ, /* num = */ 5)
to lookup the resource. That method is really simple and looks like
for (i = 0; i < dev->num_resources; i++) { struct resource *r = &dev->resource[i];
if (type == resource_type(r) && num-- == 0) return r; }
So when you request an IRQ at index=5, it loops through all resources, skips the first 5 IRQs and returns the 6th one (on my device, it returns NULL because there are not enough IRQs listed).
There is a bit more magic in platform_irq_count (it looks up all IRQs and does not count invalid ones), so to be absolutely safe we could just check platform_get_resource(IRQ, 5) == NULL early. If it returns NULL, then platform_get_irq(pdev, 5) will also return -ENXIO, so treating the device as BYT-T is guaranteed to fail. In this case, we have better chances when we assume BYT-CR.
Example patch: (I have added it in probe instead of is_byt_cr() because it requires the platform device, plus I think it might be better to differentiate the messages in the kernel log..)
--- a/sound/soc/intel/atom/sst/sst_acpi.c +++ b/sound/soc/intel/atom/sst/sst_acpi.c @@ -337,6 +337,16 @@ static int sst_acpi_probe(struct platform_device *pdev) if (!((ret < 0) || (bytcr == false))) { dev_info(dev, "Detected Baytrail-CR platform\n");
/* override resource info */ byt_rvp_platform_data.res_info = &bytcr_res_info; + } else if (platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) { + /* + * Some devices detected as BYT-T have only a single IRQ listed. + * In this case, platform_get_irq with index 5 will return -ENXIO. + * Fall back to the BYT-CR resource info to use the correct IRQ. + */ + dev_info(dev, "Falling back to Baytrail-CR platform\n");
+ /* override resource info */ + byt_rvp_platform_data.res_info = &bytcr_res_info; }
- the test would affect all existing devices, and there's so much hardware
proliferation that proving this change in harmless might be difficult. I personally only have one BYT-T (ASus T100) device left and it's not very reliable. Hans seems to have a ton of devices but they are mostly Byt-Cr?
With the updated patch above I believe there is literally no way this can break sound on any device. The condition only evaluates to true if SST would normally fail to probe later anyway.
I have tested the patch above on my device with: - as-is, without any modifications: -> "Falling back to Baytrail-CR platform", sound now working - a simulated "BYT-T" device: (copied the IRQs from the DSDT of the T100TA) -> "BYT-CR not detected" - uses 5th IRQ, sound working - a simulated "BYT-CR" device (made is_byt_cr() return "true" and copied the IRQs from the DSDT of the T100TAF) -> "Detected Baytrail-CR platform" - uses IRQ at index 0, sound working
Let me know what you think!
Sounds good, playing with resources is what I had in mind rather than an interrupt count which isn't necessarily safe. The only improvement I would suggest is to add this test inside of is_byt_cr(). This routine will be moved as a helper outside of sst_acpi to be reused for SOF, so if we can make this test more self-contained it's more future-proof.
AFAICT this will not fix all cases of this, so it might be better to see if we can make is_byt_cr() return true on these devices in some other way.
E.g. the "Teclast X98 Air 3G" Antonio reported does list 5 IRQs, but we should still use the first IRQ and not the 5t.
Antonio, do you know if your device uses SSP0 ?
Regards,
Hans
On Wed, 19 Dec 2018 15:23:13 +0100 Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 19-12-18 15:04, Pierre-Louis Bossart wrote:
On 12/19/18 7:07 AM, Stephan Gerhold wrote:
[...]
I have tested the patch above on my device with: - as-is, without any modifications: -> "Falling back to Baytrail-CR platform", sound now working - a simulated "BYT-T" device: (copied the IRQs from the DSDT of the T100TA) -> "BYT-CR not detected" - uses 5th IRQ, sound working - a simulated "BYT-CR" device (made is_byt_cr() return "true" and copied the IRQs from the DSDT of the T100TAF) -> "Detected Baytrail-CR platform" - uses IRQ at index 0, sound working
Let me know what you think!
Sounds good, playing with resources is what I had in mind rather than an interrupt count which isn't necessarily safe. The only improvement I would suggest is to add this test inside of is_byt_cr(). This routine will be moved as a helper outside of sst_acpi to be reused for SOF, so if we can make this test more self-contained it's more future-proof.
AFAICT this will not fix all cases of this, so it might be better to see if we can make is_byt_cr() return true on these devices in some other way.
E.g. the "Teclast X98 Air 3G" Antonio reported does list 5 IRQs, but we should still use the first IRQ and not the 5t.
Antonio, do you know if your device uses SSP0 ?
TBH I don't remember off the top of my head, I'll check my notes and get back on that when I also report back about recent kernels on my device.
Ciao, Antonio
Hi,
On 19-12-18 21:59, Antonio Ospite wrote:
On Wed, 19 Dec 2018 15:23:13 +0100 Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 19-12-18 15:04, Pierre-Louis Bossart wrote:
On 12/19/18 7:07 AM, Stephan Gerhold wrote:
[...]
I have tested the patch above on my device with: - as-is, without any modifications: -> "Falling back to Baytrail-CR platform", sound now working - a simulated "BYT-T" device: (copied the IRQs from the DSDT of the T100TA) -> "BYT-CR not detected" - uses 5th IRQ, sound working - a simulated "BYT-CR" device (made is_byt_cr() return "true" and copied the IRQs from the DSDT of the T100TAF) -> "Detected Baytrail-CR platform" - uses IRQ at index 0, sound working
Let me know what you think!
Sounds good, playing with resources is what I had in mind rather than an interrupt count which isn't necessarily safe. The only improvement I would suggest is to add this test inside of is_byt_cr(). This routine will be moved as a helper outside of sst_acpi to be reused for SOF, so if we can make this test more self-contained it's more future-proof.
AFAICT this will not fix all cases of this, so it might be better to see if we can make is_byt_cr() return true on these devices in some other way.
E.g. the "Teclast X98 Air 3G" Antonio reported does list 5 IRQs, but we should still use the first IRQ and not the 5t.
Antonio, do you know if your device uses SSP0 ?
TBH I don't remember off the top of my head, I'll check my notes and get back on that when I also report back about recent kernels on my device.
As Stephan pointed out the kernel has this quirk for your device:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/so...
Which says it is using SSP0, which indicates it is a Bay Trail CR (Cost Reduced) device. So it should indeed by using the IRQ at index 0, not at index 5. Chances are when you first tested things the kernel did not have special handling for the CR variant yet and things will just work.
In which case we can go with Stephan's solution of checking for there not being a 5th IRQ to recognize the BYTCR like devices which are not properly recognized as BYTCR.
Regards,
Hans
On Wed, Dec 19, 2018 at 08:04:35AM -0600, Pierre-Louis Bossart wrote:
On 12/19/18 7:07 AM, Stephan Gerhold wrote:
On Mon, Dec 17, 2018 at 08:13:36PM -0600, Pierre-Louis Bossart wrote:
> The quirks to get sound working with bytcr-rt5640 on that device are: > BYT_RT5640_SSP0_AIF1 | BYT_RT5640_IN1_MAP | BYT_RT5640_MCLK_EN > > I guess this means that SSP0 is being used? Yes indeed, and that makes me think we should force this device to look like Baytrail-CR.
You can do this with a DMI-based quirk (preferably in is_byt_cr directly so that I can reuse the code when I move it to a helper at some point).
Okay - thanks! One last question: I was looking at the ACPI DSDT tables of some similar devices and have found two others that look the same (only one IRQ listed). In this case, the BYT-T acpi_ipc_irq_index = 5 will never work, and we will definitely have a better chances with trying Baytrail-CR.
One of them actually had a similar patch proposed at [1] (although they did it in a weird way and also need an extra machine driver).
We could also detect this situation in a generic way with something like
if (platform_irq_count(pdev) == 1) { *bytcr = true;return 0; }
... instead of a DMI quirk. What do you think?
To avoid confusion: The existing PMIC-type based is_byt_cr() detection would be used in all other cases (i.e. if irq_count != 1), so it won't make any difference for the devices that are already working fine. (Most BYT-CR devices seem to have 5 IRQs listed)
So it's more like
if (platform_irq_count(pdev) == 1) { *bytcr = true; } else { // pmic-type based detection... }with platform_irq_count == 1 as condition for the "quirk".
The solution seems appealing but
- does it really work? I am not sure an index=5 means there are 5
interrupts.
Yes, I believe that it means that there need to be (at least) 5 interrupts.
I have checked the source code of platform_get_irq. When you do platform_get_irq(pdev, /* index = */ 5) (as done for BYT-T) it first calls
platform_get_resource(/* type = */ IORESOURCE_IRQ, /* num = */ 5)to lookup the resource. That method is really simple and looks like
for (i = 0; i < dev->num_resources; i++) { struct resource *r = &dev->resource[i];
if (type == resource_type(r) && num-- == 0) return r;}
So when you request an IRQ at index=5, it loops through all resources, skips the first 5 IRQs and returns the 6th one (on my device, it returns NULL because there are not enough IRQs listed).
There is a bit more magic in platform_irq_count (it looks up all IRQs and does not count invalid ones), so to be absolutely safe we could just check platform_get_resource(IRQ, 5) == NULL early. If it returns NULL, then platform_get_irq(pdev, 5) will also return -ENXIO, so treating the device as BYT-T is guaranteed to fail. In this case, we have better chances when we assume BYT-CR.
Example patch: (I have added it in probe instead of is_byt_cr() because it requires the platform device, plus I think it might be better to differentiate the messages in the kernel log..)
--- a/sound/soc/intel/atom/sst/sst_acpi.c +++ b/sound/soc/intel/atom/sst/sst_acpi.c @@ -337,6 +337,16 @@ static int sst_acpi_probe(struct platform_device *pdev) if (!((ret < 0) || (bytcr == false))) { dev_info(dev, "Detected Baytrail-CR platform\n");
/* override resource info */ byt_rvp_platform_data.res_info = &bytcr_res_info;
- } else if (platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) {
/** Some devices detected as BYT-T have only a single IRQ listed.* In this case, platform_get_irq with index 5 will return -ENXIO.* Fall back to the BYT-CR resource info to use the correct IRQ.*/dev_info(dev, "Falling back to Baytrail-CR platform\n");/* override resource info */byt_rvp_platform_data.res_info = &bytcr_res_info;
- the test would affect all existing devices, and there's so much hardware
proliferation that proving this change in harmless might be difficult. I personally only have one BYT-T (ASus T100) device left and it's not very reliable. Hans seems to have a ton of devices but they are mostly Byt-Cr?
With the updated patch above I believe there is literally no way this can break sound on any device. The condition only evaluates to true if SST would normally fail to probe later anyway.
I have tested the patch above on my device with:
- as-is, without any modifications: -> "Falling back to Baytrail-CR platform", sound now working
- a simulated "BYT-T" device: (copied the IRQs from the DSDT of the T100TA) -> "BYT-CR not detected" - uses 5th IRQ, sound working
- a simulated "BYT-CR" device (made is_byt_cr() return "true" and copied the IRQs from the DSDT of the T100TAF) -> "Detected Baytrail-CR platform" - uses IRQ at index 0, sound working
Let me know what you think!
Sounds good, playing with resources is what I had in mind rather than an interrupt count which isn't necessarily safe. The only improvement I would suggest is to add this test inside of is_byt_cr(). This routine will be moved as a helper outside of sst_acpi to be reused for SOF, so if we can make this test more self-contained it's more future-proof.
Hmm. Part of why I put it outside of is_byt_cr() is so that the pmic-type based detection always runs, and my new check is only used as a fallback. That allows us to see in the log if the device was detected as BYT-CR through the pmic detection or if BYT-CR is just used as fallback. (Might be useful for troubleshooting in the future..)
The design of is_byt_cr() with multiple returns (and even more so the SOF refactored version at [1]) makes it a bit difficult for me to apply this fallback after the detection, at least if this is supposed to be a single method.
So we can either
(1) Skip pmic-based detection if platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL:
--- a/sound/soc/intel/atom/sst/sst_acpi.c +++ b/sound/soc/intel/atom/sst/sst_acpi.c @@ -255,10 +255,18 @@ static int is_byt(void) return status; }
-static int is_byt_cr(struct device *dev, bool *bytcr) +static int is_byt_cr(struct platform_device *pdev, bool *bytcr) { + struct device *dev = &pdev->dev; int status = 0;
+ if (platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) { + /* This message is even shown if the device would be detected as BYT-CR below */ + dev_info(dev, "Falling back to Baytrail-CR platform\n"); + *bytcr = true; + return status; + } + if (IS_ENABLED(CONFIG_IOSF_MBI)) { u32 bios_status;
or (2) Rename is_byt_cr() to something like is_bytcr_pmic() and wrap it with a new method:
static int is_byt_cr(struct platform_device *pdev, bool *bytcr) { int status = is_byt_cr_pmic(&pdev->dev, bytcr); if (!*bytcr && platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) { *bytcr = true; dev_info(&pdev->dev, "Falling back to Baytrail-CR platform\n"); return 0; } return status; }
The end result is the same. The only difference is if we also log the result of the pmic-based detection.
Which one do you prefer? (Or any other suggestions?)
[1]: https://github.com/thesofproject/linux/blob/43b4e383f85446a7f43f7fd19f382d34...
-static int is_byt_cr(struct device *dev, bool *bytcr) +static int is_byt_cr(struct platform_device *pdev, bool *bytcr) {
struct device *dev = &pdev->dev; int status = 0;
if (platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) {
/* This message is even shown if the device would be detected as BYT-CR below */dev_info(dev, "Falling back to Baytrail-CR platform\n");*bytcr = true;return status;}
if (IS_ENABLED(CONFIG_IOSF_MBI)) { u32 bios_status;
This would be my preferred solution but if it doesn't work as Hans mentions it then we need to think of alternatives.
Baytrail platforms are so different (BIOS and hardware) that I don't think we'll manage to pull this off without quirks.
On Wed, Dec 19, 2018 at 10:54:55AM -0600, Pierre-Louis Bossart wrote:
-static int is_byt_cr(struct device *dev, bool *bytcr) +static int is_byt_cr(struct platform_device *pdev, bool *bytcr) {
- struct device *dev = &pdev->dev; int status = 0;
- if (platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) {
/* This message is even shown if the device would be detected as BYT-CR below */dev_info(dev, "Falling back to Baytrail-CR platform\n");*bytcr = true;return status;- }
- if (IS_ENABLED(CONFIG_IOSF_MBI)) { u32 bios_status;
This would be my preferred solution but if it doesn't work as Hans mentions it then we need to think of alternatives.
Baytrail platforms are so different (BIOS and hardware) that I don't think we'll manage to pull this off without quirks.
It definitely works on my device and the few others I have seen with only one IRQ listed. But there might be devices out there which are not covered by the pmic-type based detection but still have all 6 IRQs listed.
As for the "Teclast X98 Air 3G": Antonio, you mentioned that you have last tested mainline a few years back. Can you re-test without any modifications to the DSDT table on a recent mainline kernel?
I just wonder if it is really not covered by the pmic-type based detection. It does have quirks in mainline that were added with the pull request that also added the pmic-type based BYT-CR detection (see [1]).
[1]: http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111704.html
On Wed, 19 Dec 2018 18:35:02 +0100 Stephan Gerhold stephan@gerhold.net wrote:
On Wed, Dec 19, 2018 at 10:54:55AM -0600, Pierre-Louis Bossart wrote:
-static int is_byt_cr(struct device *dev, bool *bytcr) +static int is_byt_cr(struct platform_device *pdev, bool *bytcr) {
- struct device *dev = &pdev->dev; int status = 0;
- if (platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) {
/* This message is even shown if the device would be detected as BYT-CR below */dev_info(dev, "Falling back to Baytrail-CR platform\n");*bytcr = true;return status;- }
- if (IS_ENABLED(CONFIG_IOSF_MBI)) { u32 bios_status;
This would be my preferred solution but if it doesn't work as Hans mentions it then we need to think of alternatives.
Baytrail platforms are so different (BIOS and hardware) that I don't think we'll manage to pull this off without quirks.
It definitely works on my device and the few others I have seen with only one IRQ listed. But there might be devices out there which are not covered by the pmic-type based detection but still have all 6 IRQs listed.
As for the "Teclast X98 Air 3G": Antonio, you mentioned that you have last tested mainline a few years back. Can you re-test without any modifications to the DSDT table on a recent mainline kernel?
I'll try to boot a recent kernel with the original DSDT this Sunday, if I fail to find the time I should be able to do it on Dec 27th.
Ciao, Antonio
On Wed, 19 Dec 2018 21:56:10 +0100 Antonio Ospite ao2@ao2.it wrote:
On Wed, 19 Dec 2018 18:35:02 +0100 Stephan Gerhold stephan@gerhold.net wrote:
On Wed, Dec 19, 2018 at 10:54:55AM -0600, Pierre-Louis Bossart wrote:
[...]
Baytrail platforms are so different (BIOS and hardware) that I don't think we'll manage to pull this off without quirks.
It definitely works on my device and the few others I have seen with only one IRQ listed. But there might be devices out there which are not covered by the pmic-type based detection but still have all 6 IRQs listed.
As for the "Teclast X98 Air 3G": Antonio, you mentioned that you have last tested mainline a few years back. Can you re-test without any modifications to the DSDT table on a recent mainline kernel?
I'll try to boot a recent kernel with the original DSDT this Sunday, if I fail to find the time I should be able to do it on Dec 27th.
JFTR I managed to boot a recent kernel on the "Teclast X98 Air 3G" without overriding the DSDT, and audio works fine indeed.
The touchscreen does not (the IRQ number is wrong in DSDT), but that's another story. :)
Thank you, Antonio
participants (5)
-
Antonio Ospite -
Hans de Goede -
Mark Brown -
Pierre-Louis Bossart -
Stephan Gerhold