geode: Update OLPC power management

[Jordan Crouse]

Thanks for your comments, I really appreciate it.

I guess I should provide a bit of context for this code.  When the
5535 (the predecessor to the 5536) first came out, the BIOS folks decided
to combine some of the "random" legacy devices as bars under what 
we call the ISA_BRIDGE device, and it has been that way until this day.

This has been very frustrating, since it doesn't mesh very well with the
Linux PCI model.  To get around that - most of the drivers have either
read the resource base directly from the 5536 MSRs (such as the 
cs5535_gpio driver), and others like the SMBUS driver have probed for 
the PCI devices, but not actually used them.  The SMBUS driver is 
particularly confusing because the same block was accessible through
ISA ports in earlier generations of the Geode, so we have to do way 
more probing then one would expect.

This driver was an effort to provide some place to manage the PCI device,
particularly for power management purposes.  In the end, its like we are
designing our own API for our own bus, but we're really hijacking existing
infrastructure, which seemed easier to me then creating a new device type 
from scratch just for the Geode.

Comments inline.

On 04/03/07 00:42 +0100, Arnd Bergmann wrote:
> On Friday 02 March 2007 22:36:37 Jordan Crouse wrote:
> >
> > +#define MFGPT_NAME "geode-mfgpt"
> > +
> >  static struct {
> > -	char *name;
> > -	u32 msr;
> > -	int size;
> > -	u32 base;
> > -} lbars[] = {
> > -	{ "geode-pms",   MSR_LBAR_PMS, LBAR_PMS_SIZE, 0 },
> > -	{ "geode-acpi",  MSR_LBAR_ACPI, LBAR_ACPI_SIZE, 0 },
> > -	{ "geode-gpio",  MSR_LBAR_GPIO, LBAR_GPIO_SIZE, 0 },
> > -	{ "geode-mfgpt", MSR_LBAR_MFGPT, LBAR_MFGPT_SIZE, 0 }
> > +	const char *name;
> > +	void __iomem *base;
> > +	int (*suspend)(pm_message_t, void *);
> > +	int (*resume)(void *);
> > +	void *data;
> > +} geode_devices[] = {
> > +	{ "geode-smb" },
> > +	{ "geode-gpio" },
> > +	{ MFGPT_NAME },
> > +	{ "geode-interrupt" },
> > +	{ "geode-pms" },
> > +	{ "geode-acpi" }
> >  };
> 
> Not sure I understand this array. Are you enumerating functions of
> a pci device or bars of a device function, or something else?

These are all BARs on the same device.

> If it's functions, you should not need the list at all, just
> have different pci_driver structures for them. If it's bars,
> then it should not be called 'geode_devices'. Also, the use
> of the MFGPT_NAME macro in there looks weird.

Ok - agreed.  I was trying to be cute.

> > +		return (ioread32(base + reg) & (1 << gpio)) ? 1 :0;
> >  	else
> > -		return (inl(base + 0x80 + reg) & (1 << (gpio - 16))) ? 1 : 0;
> > +		return (ioread32(base + 0x80 + reg) & (1 << (gpio - 16))) ? 1 : 0;
> >  }
> 
> This change looks useful, but does the conversion to pci_iomap have anything
> to do with the power management implementation? Maybe that should be a 
> separate patch instead.

I figured that as long as we were consolidating the BARs and accessing them
with common functions, I might as well switch over to pci_iomap and save
ourselves the pain of trying to remember which BAR was I/O and which was
memory.  But I can probably do this in a different patch - I think all of our
existing drivers use I/O

> > +static int __init geode_southbridge_init(void) {
> > +
> > +	if (is_geode())
> > +		return pci_register_driver(&geode_dev_driver);
> > +	else
> > +		return -1;
> > +}
> > +
> > +fs_initcall(geode_southbridge_init);
> 
> fs_initcall? It does not make much sense to me, but it may be
> the best you can do, in which case it at least needs a comment.
> Have you tried subsys_initcall?

Yeah - it crashed with a 'bad magic in spinlock' bug.  I guessed that
it was because the function might be running before PCI enumeration, so
I moved it.  I know Andres is chasing the same bug in a different context,
so its possible I was being bit by that one rather then initcall ordering.
I'll comment it to explain myself, and if we can move it back to subsys
later, that would be great.

> > -static __init int scx200_scan_pci(void)
> > +static __init int scx200_do_scan(void)
> >  {
> > -	int data, dev;
> > -	int rc = -ENODEV;
> >  	struct pci_dev *pdev;
> > +	int ret;
> >
> > -	for(dev = 0; dev < ARRAY_SIZE(scx200_pci); dev++) {
> > -		pdev = pci_get_device(scx200_pci[dev].vendor,
> > -				scx200_pci[dev].device, NULL);
> > +	/* Search for a PCI device that will give us a clue as to which
> > +	   SMBUS block we are dealing with. */
> 
> Is it possible to use pci_register_driver here instead of doing
> your own scanning? That should make this driver somewhat cleaner.

Unfortunately, as I was explaining earlier, the SMBUS is just another
BAR on the ISA_BRIDGE function.  This extra ugliness is to look for the
SC1100/SC200 chips, since this block also works for those processors.
Like I said in my cover letter, I've never been very happy with the way this
driver works.

-- 
Jordan Crouse
Senior Linux Engineer
Advanced Micro Devices, Inc.
<www.amd.com/embeddedprocessors>