Re: [PATCH] pinctrl: icelake: Fix the resource number for community-4/5

From: Andy Shevchenko
Date: Mon Sep 24 2018 - 10:00:00 EST


On Thu, Sep 20, 2018 at 10:19:34AM -0700, Rajat Jain wrote:
> On Fri, Sep 14, 2018 at 2:38 PM Rajat Jain <rajatja@xxxxxxxxxx> wrote:
> > On Fri, Sep 14, 2018 at 2:06 PM Rajat Jain <rajatja@xxxxxxxxxx> wrote:
> > > On Fri, Sep 14, 2018 at 12:41 AM Andy Shevchenko
> > > <andy.shevchenko@xxxxxxxxx> wrote:
> > > > On Fri, Sep 14, 2018 at 1:54 AM Rajat Jain <rajatja@xxxxxxxxxx> wrote:
> > > > >
> > > > > The Icelake does not have a community-3, and the memory resources are
> > > > > laid out in the following order in the ACPI:
> > > > >
> > > > > resource-0: community-0 registers
> > > > > resource-1: community-1 registers
> > > > > resource-2: community-2 registers
> > > > > resource-3: community-4 registers
> > > > > resource-4: community-5 registers
> > > > >
> > > > > (EDS also describes the communities in the above order).
> > > > >
> > > > > Since the pinctrl driver exposes communities 0, 1, 4, 5, it needs to get
> > > > > the corresponding community registers by getting the resourse number right.
> > > > > Currently the resourse number is not correct for community 4 and 5, thus
> > > > > fix that.
> > > > >
> > > >
> > > > Can you share link to the ACPI dump of the tables? (you may get one by
> > > > running `acpidump -o tables.dat`)
>
> Hello Andy,
>
> Any feedback on this patch? I provided a dump of the ACPI below,
> please let me know if you need more info.

Sorry it took a while (I had to get tested on our reference BIOS(es) on Ice Lake platforms we have).
See my reply below.

> > > I don't have that command on my system, but I took
> > > /sys/firmware/acpi/tables/DSDT from the system and disassembled it
> > > using Intel disassembler (iasl -d) and here is the relevant portion
> > > that describes the GPIO controller. The port IDs for communities can
> > > be seen in the below output (i.e. the PCRB()):
> >
> > I realized PCRB() is an ACPI method defined in the same disasembly:
> >
> > Method (PCRB, 1, NotSerialized)
> > {
> > Return ((0xFD000000 + (Arg0 << 0x10)))
> > }
> >
> > >
> > > Device (GPIO)
> > > {
> > > Name (_HID, "INT3455") // _HID: Hardware ID
> > > Name (_UID, Zero) // _UID: Unique ID
> > > Name (_DDN, "GPIO Controller") // _DDN: DOS Device Name
> > > Name (RBUF, ResourceTemplate ()
> > > {
> > > Memory32Fixed (ReadWrite,
> > > 0x00000000, // Address Base
> > > 0x00000000, // Address Length
> > > _Y06)
> > > Memory32Fixed (ReadWrite,
> > > 0x00000000, // Address Base
> > > 0x00000000, // Address Length
> > > _Y07)
> > > Memory32Fixed (ReadWrite,
> > > 0x00000000, // Address Base
> > > 0x00000000, // Address Length
> > > _Y08)
> > > Memory32Fixed (ReadWrite,
> > > 0x00000000, // Address Base
> > > 0x00000000, // Address Length
> > > _Y09)
> > > Memory32Fixed (ReadWrite,
> > > 0x00000000, // Address Base
> > > 0x00000000, // Address Length
> > > _Y0A)
> > > Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
> > > {
> > > 0x0000000E,
> > > }
> > > })
> > > Method (_CRS, 0, NotSerialized) // _CRS: Current
> > > Resource Settings
> > > {
> > > CreateDWordField (RBUF, \_SB.PCI0.GPIO._Y06._BAS,
> > > BAS0) // _BAS: Base Address
> > > CreateDWordField (RBUF, \_SB.PCI0.GPIO._Y06._LEN,
> > > LEN0) // _LEN: Length
> > > BAS0 = PCRB (0x6E)
> > > LEN0 = 0x00010000
> > > CreateDWordField (RBUF, \_SB.PCI0.GPIO._Y07._BAS,
> > > BAS1) // _BAS: Base Address
> > > CreateDWordField (RBUF, \_SB.PCI0.GPIO._Y07._LEN,
> > > LEN1) // _LEN: Length
> > > BAS1 = PCRB (0x6D)
> > > LEN1 = 0x00010000
> > > CreateDWordField (RBUF, \_SB.PCI0.GPIO._Y08._BAS,
> > > BAS2) // _BAS: Base Address
> > > CreateDWordField (RBUF, \_SB.PCI0.GPIO._Y08._LEN,
> > > LEN2) // _LEN: Length
> > > BAS2 = PCRB (0x6C)
> > > LEN2 = 0x00010000
> > > CreateDWordField (RBUF, \_SB.PCI0.GPIO._Y09._BAS,
> > > BAS4) // _BAS: Base Address
> > > CreateDWordField (RBUF, \_SB.PCI0.GPIO._Y09._LEN,
> > > LEN4) // _LEN: Length
> > > BAS4 = PCRB (0x6A)
> > > LEN4 = 0x00010000
> > > CreateDWordField (RBUF, \_SB.PCI0.GPIO._Y0A._BAS,
> > > BAS5) // _BAS: Base Address
> > > CreateDWordField (RBUF, \_SB.PCI0.GPIO._Y0A._LEN,
> > > LEN5) // _LEN: Length
> > > BAS5 = PCRB (0x69)
> > > LEN5 = 0x00010000
> > > Return (RBUF) /* \_SB_.PCI0.GPIO.RBUF */
> > > }
> > >
> > > Method (_STA, 0, NotSerialized) // _STA: Status
> > > {
> > > Return (0x0F)
> > > }
> > > }
> > >
> > > Please let me know if this helps, or if you need more info.

First of all, this is pre-production chip, so, I don't think there is a bug in the driver (yet) discovered.

Looking to the above ASL code I may conclude that is definitely is *not* from our reference BIOS.
I have checked two versions of it and found that in both we have the following mapping:
for LP variant: there are only 4 communities are exported
for H variant: there are only 5 communities are exported

So, I guess the problem is in ASL code you provided. It simple should not
export that community at all.

In case you need to do so, there are ways:
- contact Intel and ask for a change in reference BIOS
- acquire another ACPI ID for the case, or, perhaps use special constants like
_HRV for that purpose (also need to contact Intel while doing that)

P.S. I think EDS covers it as it present in HW, though not exported by FW.

--
With Best Regards,
Andy Shevchenko