[alsa-devel] 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
>