ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Stephan Gerhold]

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/f48c78c11b0819b899f988407b6ece3d8c2cca71/dsdt.dsl#L3989-L4035

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Hans de Goede]

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

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Antonio Ospite]

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/f718ea7d184cf266d7b5453124139b1c627221e6

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

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Hans de Goede]

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/f718ea7d184cf266d7b5453124139b1c627221e6
> 
> 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

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Antonio Ospite]

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/f718ea7d184cf266d7b5453124139b1c627221e6
> > 
> > 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

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Stephan Gerhold]

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
> 

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1076 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Pierre-Louis Bossart]

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

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 598 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Pierre-Louis Bossart]

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
>
> [1]: https://github.com/me176c-dev/me176c-acpi/blob/f48c78c11b0819b899f988407b6ece3d8c2cca71/dsdt.dsl#L3989-L4035
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Stephan Gerhold]

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
> > 
> > [1]: https://github.com/me176c-dev/me176c-acpi/blob/f48c78c11b0819b899f988407b6ece3d8c2cca71/dsdt.dsl#L3989-L4035
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Pierre-Louis Bossart]

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
>>>
>>> [1]: https://github.com/me176c-dev/me176c-acpi/blob/f48c78c11b0819b899f988407b6ece3d8c2cca71/dsdt.dsl#L3989-L4035
>>> _______________________________________________
>>> Alsa-devel mailing list
>>> Alsa-devel@alsa-project.org
>>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Stephan Gerhold]

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/include/asm/spid.h

> 
> 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
> > > > 
> > > > [1]: https://github.com/me176c-dev/me176c-acpi/blob/f48c78c11b0819b899f988407b6ece3d8c2cca71/dsdt.dsl#L3989-L4035
> > > > _______________________________________________
> > > > Alsa-devel mailing list
> > > > Alsa-devel@alsa-project.org
> > > > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Pierre-Louis Bossart]

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.
>
> [1]: https://github.com/me176c-dev/me176c-kernel/blob/stock/kernel/arch/x86/include/asm/spid.h

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

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Stephan Gerhold]

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.
> > 
> > [1]: https://github.com/me176c-dev/me176c-kernel/blob/stock/kernel/arch/x86/include/asm/spid.h
> 
> 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
> 

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Stephan Gerhold]

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.
> > > 
> > > [1]: https://github.com/me176c-dev/me176c-kernel/blob/stock/kernel/arch/x86/include/asm/spid.h
> > 
> > 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".

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

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Pierre-Louis Bossart]

>>>> 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?

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Stephan Gerhold]

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
> 
> 1) 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;
 	}

> 
> 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?
> 

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!

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Pierre-Louis Bossart]

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
>>
>> 1) 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;
>   	}
> 
>>
>> 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?
>>
> 
> 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.

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Hans de Goede]

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
>>>
>>> 1) 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;
>>       }
>>
>>>
>>> 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?
>>>
>>
>> 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


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Antonio Ospite]

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

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Hans de Goede]

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/sound/soc/intel/boards/bytcr_rt5640.c?id=ec1c90e777e5a555632747527fae142aa238e583

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


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Stephan Gerhold]

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
> > > 
> > > 1) 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;
> >   	}
> > 
> > > 
> > > 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?
> > > 
> > 
> > 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/43b4e383f85446a7f43f7fd19f382d344afb7d62/sound/soc/sof/sof-acpi-dev.c#L75-L105

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Pierre-Louis Bossart]

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

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Stephan Gerhold]

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

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Antonio Ospite]

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

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: ASoC: Intel: sst: Missing IRQ at index 5 on BYT-T device

[Antonio Ospite]

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

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?