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@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 = OnePSTS = 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.