[U-Boot] [PATCH v1 1/2] x86: tangier: Enable ACPI support for Intel Tangier

[Andy Shevchenko]

On Sun, 2017-09-03 at 17:46 +0800, Bin Meng wrote:
> Hi Andy,
> 
> On Wed, Aug 30, 2017 at 11:04 PM, Andy Shevchenko
> <andriy.shevchenko at linux.intel.com> wrote:
> > Intel Tangier SoC is a part of Intel Merrifield platform which
> > doesn't
> > utilize ACPI by default. Here is an attempt to unleash ACPI
> > flexibility
> > power on Intel Merrifield based platforms.
> > 
> > The change brings minimum support of the devices that found on
> > Intel Merrifield based end user device.
> > 

> Generally it looks good. Some comments below.

Thanks for review, my answers below.

> > +u32 acpi_fill_mcfg(u32 current)
> > +{
> > +       current += acpi_create_mcfg_mmconfig
> > +               ((struct acpi_mcfg_mmconfig *)current,
> > +               0x3f500000, 0x0, 0x0, 0x0);
> 
> What is 0x3f500000?

Nice question (I have in my local branch an additional commit with FIXME
near to this line)!

The root cause is a hardware design here. There are two (okay, on those
platforms up to four) *real* PCI devices. The rest have PCI
*programming* interface but being not PCI at the same time. This is a
magic address of so called PCI Shim for those (non-PCI) devices which
filed by firmware.

>  Can we define something in asm/arch/iomap.h (like
> Baytrail) and use it here?

It comes from SFI, so, the best approach is to parse SFI for that.

I would not do this right now (will take a lot of time for not much
benefit), though can put as TODO.

>  And I see the end_bus_number is set to zero
> here, why?

See above.

>  Is this table a faked one?

It's based on SFI.

>  How about completely drop this
> table? Does Linux boot without this table?

It might start booting (as initial part), but it will lose an ability to
enumerate almost all devices in SoC. Basically user will not get even
console.

> > +
> > +       return current;
> > +}
> > 

> > +OperationRegion(GNVS, SystemMemory, ACPI_GNVS_ADDR, ACPI_GNVS_SIZE)
> > +Field(GNVS, ByteAcc, NoLock, Preserve)
> > +{
> > +    Offset (0x00),
> > +    PCNT, 2,    /* processor count */
> 
> 2 is weird here. Why only two bits? I believe it should be 8 per your
> global_nvs.h defines.

Ah, I misread what that number means. Will fix.

> > +        Method (_PS0, 0, NotSerialized)
> > +        {
> > +            If (PSTS == Zero)
> > +            {
> 
> nits: can we do something similar to U-Boot coding style with these
> If/Else?

I would rather follow iasl style for ASL.

Though if it's obliged to use U-Boot style over all files, I can switch.

> 
> > +                If (^^GPIO.AVBL == One)
> > +                {
> > +                    ^^GPIO.WFD3 = One
> > +                    PSTS = One
> > +                }
> > +            }
> > +        }
> > +
> > +        /* BCM43340 */
> > +        Device (BRC1)
> > +        {
> > +            Name (_ADR, One)
> 
> nits: since it represents an address, I would use 0x01 instead of One

I have no strong opinion here, ACPI spec says

"The use of this operator can reduce AML code size, since it is
represented by a one-byte AML opcode."

Same for Zero.

Though to be consistent with the rest, I will change it.

> +        Device (BRC2)
> > +        {
> > +            Name (_ADR, 0x02)
> > 

> > +        Name (RBUF, ResourceTemplate()
> > +        {
> > +            GpioIo(Exclusive, PullUp, 0, 0,
> > IoRestrictionOutputOnly,
> > +                "\\_SB.PCI0.GPIO", 0, ResourceConsumer, , ) { 91 }
> 
> nits: can we use relative path here, like "GPIO"?

While spec says:

"ResourceSource can be a fully-qualified name, a relative name or a name
segment that utilizes the namespace search rules."

I would rather follow common practice, i.e. to use fully-qualified name
for GPIO and Pin control resources.

> 
> > +            GpioIo(Exclusive, PullUp, 0, 0,
> > IoRestrictionOutputOnly,
> > +                "\\_SB.PCI0.GPIO", 0, ResourceConsumer, , ) { 92 }
> > +            GpioIo(Exclusive, PullUp, 0, 0,
> > IoRestrictionOutputOnly,
> > +                "\\_SB.PCI0.GPIO", 0, ResourceConsumer, , ) { 93 }
> > +            GpioIo(Exclusive, PullUp, 0, 0,
> > IoRestrictionOutputOnly,
> > +                "\\_SB.PCI0.GPIO", 0, ResourceConsumer, , ) { 94 }
> > 

> > +    Device (GPIO)
> > +    {
> > +        Name (_ADR, 0x000c0000)
> > +        Name (_DEP, Package (0x01)
> > +        {
> > +            \_SB.FLIS
> > +        })
> 
> Looks _DEP method is supported only for Operation Regions as I read
> from ACPI spec? Does it work here?

No, it doesn't (yet?). I would move it out to my debugging stuff.

> > +
> > +        Method (_STA)
> > +        {
> > +            Return (STA_VISIBLE)
> > +        }
> > +
> > +        Name (AVBL, Zero)
> > +        Method (_REG, 2, NotSerialized)
> > +        {
> > +            If (Arg0 == 0x08)
> 
> I assume 0x08 here means GeneralPurposeIO which is one of Operation
> Region Address Space Identifiers Values? If yes, could we add
> something in arch/x86/include/asm/acpi/ and use macro here?

This part I got based on disassemble from iasl. Moreover, there are two
scopes for this value: inside OperationRegion() and outside. Since iasl
does not replace it with such (existing) value, I would follow it rather
than creating a potential collision with names and scopes.

> > +    Name (_DSD, Package () {
> > +        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> 
> Is this new UUID supposed to describe pinctrl? I don't see it in
> include/acpi/acuuid.h in the kernel tree.

Nope, this UUID is supposed to describe device properties.

Please, refer to 6.2.5 _DSD (Device Specific Data) in the spec.

> > +        Package () {
> > +            Package () {"compatible", Package ()
> > {"intel,merrifield-pinctrl"}},
> 
> Is the 2nd Package() necessary? Like just below, is it OK?
> Package () {"compatible", "intel,merrifield-pinctrl"},

In this case yes, we have only one value for the "compatible" key.
I will change it.

> I believe the intel,merrifield-pinctrl driver is an OF driver now,

No.

The only way for now to get it enumerated via ACPI is to apply
compatible string.

> but I don't see such string in current kernel tree.

I sent recently a v2 which adds such binding to the kernel sources.

>  ACPI v6.2 has native
> support of pinctrl, but kernel is not ready for it, so this is a
> temporary placeholder?

The first part of sentence has nothing to do with the question at the
second.

Kernel is not ready for pin control glue layer (ACPICA already handles
that), but it is orthogonal to compatible strings.

-- 
Andy Shevchenko <andriy.shevchenko at linux.intel.com>
Intel Finland Oy