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

["Banik, Subrata" <subrata.banik@xxxxxxxxx>]

HI Andy,

You are right that this ASL code is not same with Intel reference BIOS code because BIOS sources are different between what you are looking vs what Chrome OS is using. In Coreboot BIOS, we are more relying on EDS spec and as COM 3 is dedicated for CPU GPIO hence we are mapping, 0/1/2/4/5 (whatever present in EDS vol 1.1)

We have ensured that PCR ID and offsets are correct in ASL code for respective community, I don't think anything else really matter from BIOS unless you tell me, that you are having any required that your drive code will only probe for 4 COMM for LP and 5 for ICL-H?

Thanks,
Subrata

-----Original Message-----
From: Andy Shevchenko [mailto:andy.shevchenko@xxxxxxxxx] 
Sent: Monday, September 24, 2018 7:30 PM
To: Rajat Jain <rajatja@xxxxxxxxxx>
Cc: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>; Linus Walleij <linus.walleij@xxxxxxxxxx>; linux-gpio@xxxxxxxxxxxxxxx; Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>; Rajat Jain <rajatxjain@xxxxxxxxx>; Banik, Subrata <subrata.banik@xxxxxxxxx>; Bohra, Aamir <aamir.bohra@xxxxxxxxx>
Subject: Re: [PATCH] pinctrl: icelake: Fix the resource number for community-4/5

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