piix4-poweroff.c I/O BAR usage

piix4-poweroff.c I/O BAR usage

[Bjorn Helgaas]

Hi Paul,

This looks like it might be a bug:

  static const int piix4_pm_io_region = PCI_BRIDGE_RESOURCES;

  static int piix4_poweroff_probe(struct pci_dev *dev,
                                  const struct pci_device_id *id)
  {
          ...
          /* Request access to the PIIX4 PM IO registers */
          res = pci_request_region(dev, piix4_pm_io_region,
                                   "PIIX4 PM IO registers");

pci_request_region() takes a BAR number (0-5), but here we're passing
PCI_BRIDGE_RESOURCES (13 if CONFIG_PCI_IOV, or 7 otherwise), which is
the bridge I/O window.

I don't think this device ([8086:7113]) is a bridge, so that resource
should be empty.

Based on this spec:
https://www.intel.com/Assets/PDF/datasheet/290562.pdf,
it looks like it should be the PIIX4 power management function at
function 3, which has no standard PCI BARs but does have a PMBA (Power
Management Base Address) at 0x40 and an SMBBA (SMBus Base Address) at
0x90 in config space.

I suppose on an ACPI system the regions described by PMBA and SMBBA
might be described via ACPI, since they're not discoverable by
standard PCI enumeration?  Pretty sure you don't have ACPI on MIPS
though.

Maybe the driver should read PMBA and SMBBA and reserve those regions
by hand with request_region()?

Bjorn

Re: piix4-poweroff.c I/O BAR usage

[Maciej W. Rozycki]

Hi Bjorn,

 Paul may or may not be reachable anymore, so I'll step in.

> This looks like it might be a bug:
> 
>   static const int piix4_pm_io_region = PCI_BRIDGE_RESOURCES;
> 
>   static int piix4_poweroff_probe(struct pci_dev *dev,
>                                   const struct pci_device_id *id)
>   {
>           ...
>           /* Request access to the PIIX4 PM IO registers */
>           res = pci_request_region(dev, piix4_pm_io_region,
>                                    "PIIX4 PM IO registers");
> 
> pci_request_region() takes a BAR number (0-5), but here we're passing
> PCI_BRIDGE_RESOURCES (13 if CONFIG_PCI_IOV, or 7 otherwise), which is
> the bridge I/O window.
> 
> I don't think this device ([8086:7113]) is a bridge, so that resource
> should be empty.

 Hmm, isn't the resource actually set up by `quirk_piix4_acpi' though?

> Based on this spec:
> https://www.intel.com/Assets/PDF/datasheet/290562.pdf,
> it looks like it should be the PIIX4 power management function at
> function 3, which has no standard PCI BARs but does have a PMBA (Power
> Management Base Address) at 0x40 and an SMBBA (SMBus Base Address) at
> 0x90 in config space.

 Correct, this is what Malta firmware reports for this function:

Bus = 0x00, Dev = 0x0a, Function = 0x03
Vendor Id = 0x8086 (Intel), Dev ID = 0x7113 (PIIX4 Power)
 Min Gnt = 0x00, Max Lat = 0x00, Lat Tim = 0x20
 Int Pin = None, Int Line = 0x09
 BAR count = 0x02
  IO:  Pos = 0x40, Base(CPU/PCI) = 0x18001000/0x00001000, Size = 0x00000100
  IO:  Pos = 0x90, Base(CPU/PCI) = 0x18001100/0x00001100, Size = 0x00000100

I'm somewhat familiar with this southbridge, although this was looong ago.

> I suppose on an ACPI system the regions described by PMBA and SMBBA
> might be described via ACPI, since they're not discoverable by
> standard PCI enumeration?  Pretty sure you don't have ACPI on MIPS
> though.
> 
> Maybe the driver should read PMBA and SMBBA and reserve those regions
> by hand with request_region()?

 Well, I think `quirk_piix4_acpi' covers it.  It dates back to 2.3.49 
AFAICT.  I can try to boot my Malta system over the weekend to see if 
there are any issues with it, but I'm fairly sure there is none here.

  Maciej

Re: piix4-poweroff.c I/O BAR usage

[Paul Burton]

Hello,

On Thu, May 21, 2020 at 6:04 PM Maciej W. Rozycki <macro@wdc.com> wrote:
>  Paul may or may not be reachable anymore, so I'll step in.

I'm reachable but lacking free time & with no access to Malta hardware
I can't claim to be too useful here, so thanks for responding :)

Before being moved to a driver (which was mostly driven by a desire to
migrate Malta to a multi-platform/generic kernel using DT) this code
was part of arch/mips/mti-malta/ where I added it in commit
b6911bba598f ("MIPS: Malta: add suspend state entry code"). My main
motivation at the time was to make QEMU exit after running poweroff,
but I did ensure it worked on real Malta boards too (at least Malta-R
with CoreFPGA6). Over the years since then it shocked a couple of
hardware people to see software power off a Malta - if the original
hardware designers had intended that to work then the knowledge had
been lost over time :)

I suspect the code was based on visws_machine_power_off():

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/platform/visws/visws_quirks.c?h=v3.10#n125

> > pci_request_region() takes a BAR number (0-5), but here we're passing
> > PCI_BRIDGE_RESOURCES (13 if CONFIG_PCI_IOV, or 7 otherwise), which is
> > the bridge I/O window.
> >
> > I don't think this device ([8086:7113]) is a bridge, so that resource
> > should be empty.
>
>  Hmm, isn't the resource actually set up by `quirk_piix4_acpi' though?

I agree that the region used is meant to match that set up by
quirk_piix4_acpi(), which also refers to it using the
PCI_BRIDGE_RESOURCES macro.

Thanks,
    Paul

Re: piix4-poweroff.c I/O BAR usage

[Maciej W. Rozycki]

On Thu, 21 May 2020, Paul Burton wrote:

> I'm reachable but lacking free time & with no access to Malta hardware
> I can't claim to be too useful here, so thanks for responding :)

 Great you're still around!  I hope you're doing good in this difficult 
time, and overall.  I have recently learnt WaveComp is no more. :(

> Before being moved to a driver (which was mostly driven by a desire to
> migrate Malta to a multi-platform/generic kernel using DT) this code
> was part of arch/mips/mti-malta/ where I added it in commit
> b6911bba598f ("MIPS: Malta: add suspend state entry code"). My main
> motivation at the time was to make QEMU exit after running poweroff,
> but I did ensure it worked on real Malta boards too (at least Malta-R
> with CoreFPGA6). Over the years since then it shocked a couple of
> hardware people to see software power off a Malta - if the original
> hardware designers had intended that to work then the knowledge had
> been lost over time :)

 Well, the Malta was designed by the Copenhagen team, which was dissolved 
IIRC back in 2003, so indeed the intent may have been lost in the mist of 
time.  I did know powering off is possible (same with the Atlas CompactPCI 
board if you ever came across one, not supported by Linux anymore) as it 
was inherent to the southbridge, and I remember discussing it once with 
Chris Shaw back in the MIPS UK days.  I guess the further it went, the 
more it became forgotten.

 Thanks for confirming it has been verified, though I was sure you did it 
anyway.

  Maciej