Message-ID: <f850063d-78c1-4fa7-b7ba-49fb4aa28c5b@zimbra>
Date:	Mon, 20 Feb 2012 14:36:27 -0600 (CST)
From:	Aaron Sierra <asierra@...-inc.com>
To:	Jean Delvare <khali@...ux-fr.org>
Cc:	Guenter Roeck <guenter@...ck-us.net>,
	Peter Tyser <ptyser@...-inc.com>,
	Grant Likely <grant.likely@...retlab.ca>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3 v4] mfd: Add LPC driver for Intel ICH chipsets

> > +{
> > +	cell->id = id->driver_data;
> > +	cell->platform_data = &lpc_chipset_info[id->driver_data];
> > +	cell->pdata_size = sizeof(struct lpc_ich_info);
> > +}
> > +
> > +static int __devinit lpc_ich_probe(struct pci_dev *dev,
> > +				const struct pci_device_id *id)
> > +{
> > +	u32 base_addr_cfg;
> > +	u32 base_addr;
> > +	u8 reg_save;
> > +	int ret;
> > +	bool cell_added = false;
> > +	bool acpi_conflict = false;
> > +
> > +	/* Setup power management base register */
> > +	pci_read_config_dword(dev, ACPIBASE, &base_addr_cfg);
> > +	base_addr = base_addr_cfg & 0x0000ff80;
> > +	if (!base_addr) {
> > +		dev_err(&dev->dev, "I/O space for ACPI uninitialized\n");
> > +		goto pm_done;
> > +	}
> > +
> > +	gpio_ich_res[ICH_RES_GPE0].start = base_addr + ACPIBASE_GPE_OFF;
> > +	gpio_ich_res[ICH_RES_GPE0].end = base_addr + ACPIBASE_GPE_END;
> > +	ret = acpi_check_resource_conflict(&gpio_ich_res[ICH_RES_GPE0]);
> > +	if (ret) {
> > +		/* this isn't necessarily fatal for the GPIO */
> > +		gpio_ich_res[ICH_RES_GPE0].start = 0;
> > +		gpio_ich_res[ICH_RES_GPE0].end = 0;
> 
> Is it really sufficient to disable the resource? I see that you handle
> this case properly in the gpio-ich driver, however there's also the
> platform subsystem which needs to be considered. The above will cause
> platform_device_add_resources (called by mfd_add_device) to register
> an I/O resource at address 0, size 1. I can see it in /proc/ioports:
> 
> 0000-0cf7 : PCI Bus 0000:00
>   0000-001f : dma1
>     0000-0000 : gpio_ich.32	<-- HERE
>   0020-0021 : pic1
> 
> This is not clean and could cause a conflict on its own. So I don't
> think this is the right approach. See below for a possible solution.

I will work on a solution for this.

> > +		acpi_conflict = true;
> 
> Don't you want to jump to pm_done here? There's no point in enabling
> the LPC ACPI space if you are never going to access it. Not that it
> should really make a difference in practice, I presume that if ACPI
> is using the resource, the LPC ACPI space is already enabled...

I think this is another case where the code structure is based on the
complete 3-patch set. In this patch it would makes sense to have a jump
here, but after the 3rd patch is applied it would be removed anyway
because I consider the GPE region to be not strictly mandatory (which I
get into more below) and the watchdog timer regions in ACPI space need
to be tested before jumping.

> > +	}
> > +
> > +	/* Enable LPC ACPI space */
> > +	pci_read_config_byte(dev, ACPICTRL, &reg_save);
> > +	pci_write_config_byte(dev, ACPICTRL, reg_save | 0x10);
> > +	lpc_ich_acpi_save = reg_save;
> > +
> > +pm_done:
> > +	/* Setup GPIO base register */
> > +	pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
> > +	base_addr = base_addr_cfg & 0x0000ff80;
> > +	if (!base_addr) {
> > +		dev_err(&dev->dev, "I/O space for GPIO uninitialized\n");
> > +		/* GPIO in power-management space may still be available */
> > +		goto gpio_reg;
> > +	}
> > +
> > +	gpio_ich_res[ICH_RES_GPIO].start = base_addr;
> > +	gpio_ich_res[ICH_RES_GPIO].end = base_addr + GPIOBASE_IO_SIZE -
> > 1;
> > +	ret = acpi_check_resource_conflict(&gpio_ich_res[ICH_RES_GPIO]);
> > +	if (ret) {
> > +		/* this isn't necessarily fatal for the GPIO */
> > +		gpio_ich_res[ICH_RES_GPIO].start = 0;
> > +		gpio_ich_res[ICH_RES_GPIO].end = 0;
> 
> I don't quite get how this can be non-fatal, given that the gpio-ich
> driver's probe function will return -ENODEV in this case. So if this
> resource is mandatory, let's make it exactly that.

The necessity for the ICH_RES_GPIO resource to exist is an issue I
thought better left to the gpio-ich driver. The way that driver is
currently written it is mandatory, but it doesn't *have* to be written
that way. For chipsets that have GPE0 and GPIO space, only one needs
to be present to have some usable GPIO.

> This means that resource 0 is mandatory and resource 1 is optional.
> All you have to do then is:
> * Don't register the mfd device at all if GPIO resource is
> unavailable.
> * If ACPI resource is unavailable, set num_resources to 1.
> 
> That should work, and this solves the ghost resource problem I
> mentioned earlier.
>
> Yet a completely different approach would be to delegate the ACPI
> resource conflict checking to the gpio-ich subdriver. I suspect we
> may end up doing that anyway, as requesting the whole I/O range when
> we only need subsets thereof is likely to cause ACPI resource
> conflicts on too many systems for the driver to be useful in practice.
> This is a bigger change though and I would understand if you are
> reluctant to do it as this point of the review cycle. This can be
> changed later and I volunteer to take care of it (I need it for my
> Asus Z8NA-D6 board.)

You mean creating resources for individual bytes within 32-bit registers?
Otherwise, the only optimization (fix) I see is that ACPIBASE_GPE0_BASE
should be 0x28, not 0x20 and gpe0_sts_ofs in gpio-ich should be removed.
Currently, that portion of gpio-ich appears to be broken.

The whole issue of anything related to boot firmware having dominion
over hardware after booting to an OS is very frustrating.

> > +		acpi_conflict = true;
> > +		goto gpio_reg;
> > +	}
> > +
> > +	/* Enable LPC GPIO space */
> > +	pci_read_config_byte(dev, GPIOCTRL, &reg_save);
> > +	pci_write_config_byte(dev, GPIOCTRL, reg_save | 0x10);
> > +	lpc_ich_gpio_save = reg_save;
> > +
> > +gpio_reg:
> 
> Shouldn't this label be named gpio_done for consistency? Probably a
> moot point given my remark above anyway.

Again, the label names come from lpc_ich after all three patches have
been applied. In that case one label jumps to code immediately following
WDT cell registration, "done with registration" and the other jumps
immediately before GPIO cell registration, "do registration". If I make
ICH_RES_GPIO mandatory, like you suggest, these labels could both follow
cell registration and be named consistently.
 
> > +	lpc_ich_finalize_cell(&lpc_ich_cells[LPC_GPIO], id);
> > +	ret = mfd_add_devices(&dev->dev, 0, &lpc_ich_cells[LPC_GPIO],
> > +				1, NULL, 0);
> > +	if (!ret)
> > +		cell_added = true;
> > +
> > +	if (acpi_conflict)
> > +		dev_info(&dev->dev, "ACPI resource conflicts found; "
> > +				"consider using acpi_enforce_resources=lax?\n");
> 
> I'm not sure if it really makes sense to report this. ACPI resource
> conflicts are already reported quite loudly by the acpi core. And
> passing acpi_enforce_resources=lax blindly isn't quite recommended,
> so I'm not sure if we really want to mention it here, it might do more
> harm than help.

So the question mark doesn't imply strongly enough that it's not an action
that should definitely be taken. Would you prefer a warning summarizing which
drivers are affected by the detected resource conflicts or no additional warning
at all?

-Aaron S.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists