On Sat, Apr 19, 2014 at 04:39:10PM +0100, Maciej W. Rozycki wrote:
> On Fri, 21 Mar 2014, Paul Burton wrote:
>
> > This patch makes the mips_machine_halt function (used as _machine_halt &
> > pm_power_off) actually power down the Malta via the PIIX4. It may then
> > be powered back up by pressing the "ON/NMI" button (S4) on the board.
> >
> > Tested-by: James Hogan <james.hogan@xxxxxxxxxx>
> > Signed-off-by: Paul Burton <paul.burton@xxxxxxxxxx>
> > ---
> [...]
> > diff --git a/arch/mips/mti-malta/malta-reset.c b/arch/mips/mti-malta/malta-reset.c
> > index d627d4b..ef04c8b 100644
> > --- a/arch/mips/mti-malta/malta-reset.c
> > +++ b/arch/mips/mti-malta/malta-reset.c
> > @@ -24,10 +27,72 @@ static void mips_machine_restart(char *command)
> >
> > static void mips_machine_halt(void)
> > {
>
> First of all, shouldn't all of this stuff be wired into
> mips_machine_power_off rather than mips_machine_halt? I would have
> thought mips_machine_halt is supposed to get back to the console monitor
> prompt (YAMON in this case) without restarting or powering off the system,
> and if that's impossible, then loop indefinitely (that's the -H vs -P
> action option to shutdown(8); see the details in the manual).
>
Perhaps. Currently there is no mips_machine_power_off - both
_machine_halt & pm_power_off point at mips_machine_halt and its existing
behaviour is to reset the system. That is, regardless of whether you
intend to halt, power off or reset you always get a reset. Returning to
the monitor/bootloader prompt (which may or may not be YAMON) is not
generally possible since the memory it was using has probably been
overwritten. It may make sense to separate halt & power off though, with
halt simply executing an infinite loop as you suggest.
> > - unsigned int __iomem *softres_reg =
> > - ioremap(SOFTRES_REG, sizeof(unsigned int));
> > + struct pci_bus *bus;
> > + struct pci_dev *dev;
> > + int spec_devid, res;
> > + int io_region = PCI_BRIDGE_RESOURCES;
> > + resource_size_t io;
> > + u16 sts;
> >
> > - __raw_writel(GORESET, softres_reg);
> > + /* Find the PIIX4 PM device */
> > + dev = pci_get_subsys(PCI_VENDOR_ID_INTEL,
> > + PCI_DEVICE_ID_INTEL_82371AB_3, PCI_ANY_ID,
> > + PCI_ANY_ID, NULL);
> > + if (!dev) {
> > + printk("Failed to find PIIX4 PM\n");
> > + goto fail;
> > + }
> > +
> > + /* Request access to the PIIX4 PM IO registers */
> > + res = pci_request_region(dev, io_region, "PIIX4 PM IO registers");
> > + if (res) {
> > + printk("Failed to request PIIX4 PM IO registers (%d)\n", res);
> > + goto fail_dev_put;
> > + }
>
> Shouldn't the handle on the device and the resource be requested early
> on, where mips_machine_halt (mips_machine_power_off) is installed as the
> halt (power-off) handler? Especially requesting the resource here seems
> to make little sense to me -- we're about to kill the box, so why bother
> verifying whether it's going to interfere with a random driver?
Well requesting the I/O region was more about sanity checking that it's
present. This could be done earlier I guess, it would just mean keeping
around the needed I/O & PCI bus pointers in globals and I don't see the
issue with just acquiring them when they're needed.
>
> > +
> > + /* Find the offset to the PIIX4 PM IO registers */
> > + io = pci_resource_start(dev, io_region);
> > +
> > + /* Ensure the power button status is clear */
> > + while (1) {
> > + sts = inw(io + PIIX4_FUNC3IO_PMSTS);
> > + if (!(sts & PIIX4_FUNC3IO_PMSTS_PWRBTN_STS))
> > + break;
> > + outw(sts, io + PIIX4_FUNC3IO_PMSTS);
> > + }
> > +
> > + /* Enable entry to suspend */
> > + outw(PIIX4_FUNC3IO_PMCNTRL_SUS_EN, io + PIIX4_FUNC3IO_PMCNTRL);
> > +
> > + /* If the special cycle occurs too soon this doesn't work... */
> > + mdelay(10);
> > +
> > + /* Find a reference to the PCI bus */
> > + bus = pci_find_next_bus(NULL);
> > + if (!bus) {
> > + printk("Failed to find PCI bus\n");
> > + goto fail_release_region;
> > + }
> > +
> > + /*
> > + * The PIIX4 will enter the suspend state only after seeing a special
> > + * cycle with the correct magic data on the PCI bus. Generate that
> > + * cycle now.
> > + */
> > + spec_devid = PCI_DEVID(0, PCI_DEVFN(0x1f, 0x7));
> > + pci_bus_write_config_dword(bus, spec_devid, 0, PIIX4_SUSPEND_MAGIC);
>
> I know all the three of the GT-64120/64120A, Bonito and SOC-it system
> controllers support software generation of PCI special cycles, but is the
> method the same across them all?
>
> Maciej
It's the same for the Galileo GT-64120 & the SOC-it/ROC-it/ROC-it2
system controllers. I'm unsure about Bonito, however there is the
fallback to the existing reset behaviour if it fails. Working on the
GT-64120 & SOC-it+derivatives covers the cases in wide use, ie current
physical Malta systems & QEMU (although QEMU doesn't seem to need the
special cycle anyway).
Paul
Attachment:
signature.asc
Description: Digital signature
