[PATCH v2 0/4] PCI/quirks: PIIX4 ACPI bugfix and cleanups

[PATCH v2 0/4] PCI/quirks: PIIX4 ACPI bugfix and cleanups

[Deng-Cheng Zhu]

Fix the PIIX4 ACPI memory base and size mask, perform cleanups.

CHANGES:
----------
v2 - v1:
o Split renaming, refactoring and macros into 3 patches.
----------

Deng-Cheng Zhu (4):
  PCI/quirks: Fix PIIX4 memory base and size mask
  PCI/quirks: Rename piix4_[io|mem]_quirk to add acpi
  PCI/quirks: Extract the size detection logic of PIIX4 ACPI io and mem
  PCI/quirks: Convert hard-coded values to macros for PIIX4 ACPI quirks

 drivers/pci/quirks.c |  116 ++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 84 insertions(+), 32 deletions(-)

[PATCH v2 1/4] PCI/quirks: Fix PIIX4 memory base and size mask

[Deng-Cheng Zhu]

From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>

According to Intel PIIX4 datasheet: The memory address consists of a 17-bit
base address (AD[31:15]) and a 7-bit mask (AD[21:15]). This provides memory
ranges from 32 Kbytes to 4 Mbytes in size.

This is true for both devices 12 and 13. This patch fixes it.

Ref 1: www.intel.com/assets/pdf/datasheet/290562.pdf
       April 1997 version 001 (p131 bottom, p226 top)
Ref 2: www.intel.com/assets/pdf/specupdate/297738.pdf
       January 2002 version 017 (Nothing against the fix in the
       specification update was found.)

Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
Reviewed-by: James Hogan <james.hogan@imgtec.com>
---
 drivers/pci/quirks.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f6c31fa..3e7e489 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -414,9 +414,9 @@ static void piix4_mem_quirk(struct pci_dev *dev, const char *name, unsigned int
 	pci_read_config_dword(dev, port, &devres);
 	if ((devres & enable) != enable)
 		return;
-	base = devres & 0xffff0000;
-	mask = (devres & 0x3f) << 16;
-	size = 128 << 16;
+	base = devres & 0xffff8000;
+	mask = (devres & 0x7f) << 15;
+	size = 128 << 15;
 	for (;;) {
 		unsigned bit = size >> 1;
 		if ((bit & mask) == bit)
-- 
1.7.1

Re: [PATCH v2 1/4] PCI/quirks: Fix PIIX4 memory base and size mask

[Bjorn Helgaas]

On Fri, Oct 4, 2013 at 5:30 PM, Deng-Cheng Zhu <dengcheng.zhu@imgtec.com> wrote:
> From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>
> According to Intel PIIX4 datasheet: The memory address consists of a 17-bit
> base address (AD[31:15]) and a 7-bit mask (AD[21:15]). This provides memory
> ranges from 32 Kbytes to 4 Mbytes in size.
>
> This is true for both devices 12 and 13. This patch fixes it.

Does this fix a user-visible problem?  If so, what does it look like
when the problem occurs?

I assume the other patches (2-4) are cleanups that don't fix any problems.

> Ref 1: www.intel.com/assets/pdf/datasheet/290562.pdf
>        April 1997 version 001 (p131 bottom, p226 top)
> Ref 2: www.intel.com/assets/pdf/specupdate/297738.pdf
>        January 2002 version 017 (Nothing against the fix in the
>        specification update was found.)
>
> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
> Reviewed-by: James Hogan <james.hogan@imgtec.com>
> ---
>  drivers/pci/quirks.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index f6c31fa..3e7e489 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -414,9 +414,9 @@ static void piix4_mem_quirk(struct pci_dev *dev, const char *name, unsigned int
>         pci_read_config_dword(dev, port, &devres);
>         if ((devres & enable) != enable)
>                 return;
> -       base = devres & 0xffff0000;
> -       mask = (devres & 0x3f) << 16;
> -       size = 128 << 16;
> +       base = devres & 0xffff8000;
> +       mask = (devres & 0x7f) << 15;
> +       size = 128 << 15;
>         for (;;) {
>                 unsigned bit = size >> 1;
>                 if ((bit & mask) == bit)
> --
> 1.7.1
>
>

RE: [PATCH v2 1/4] PCI/quirks: Fix PIIX4 memory base and size mask

[DengCheng Zhu]

> Does this fix a user-visible problem?  If so, what does it look like
> when the problem occurs?

No, I found the problem while debugging another issue and trying to
understand this piece of code and after looking into different versions of
PIIX4 datasheets. But I don't think it should prevent such a fix because
the code is straightforward and the spec is clear enough. If the existing
encoding was intentionally made like this by the code author due to the
inaccuracy of the spec, then it's very likely some code comments were
placed here. There are 2 possibilities:

- This fix breaks something. People should have to bisect the problem.
- This fix is valid. Some day people need PIIX4 mem quirks and they don't
  have to run into a possibly well-hidden issue.
  
What do you think?

> I assume the other patches (2-4) are cleanups that don't fix any
> problems.

Definitely, as indicated by patch titles and commit messages.


Deng-Cheng

________________________________________
From: Bjorn Helgaas [bhelgaas@google.com]
Sent: Friday, October 04, 2013 7:13 PM
To: DengCheng Zhu
Cc: linux-pci@vger.kernel.org; James Hogan; Qais Yousef
Subject: Re: [PATCH v2 1/4] PCI/quirks: Fix PIIX4 memory base and size mask

On Fri, Oct 4, 2013 at 5:30 PM, Deng-Cheng Zhu <dengcheng.zhu@imgtec.com> wrote:
> From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>
> According to Intel PIIX4 datasheet: The memory address consists of a 17-bit
> base address (AD[31:15]) and a 7-bit mask (AD[21:15]). This provides memory
> ranges from 32 Kbytes to 4 Mbytes in size.
>
> This is true for both devices 12 and 13. This patch fixes it.

Does this fix a user-visible problem?  If so, what does it look like
when the problem occurs?

I assume the other patches (2-4) are cleanups that don't fix any problems.

> Ref 1: www.intel.com/assets/pdf/datasheet/290562.pdf
>        April 1997 version 001 (p131 bottom, p226 top)
> Ref 2: www.intel.com/assets/pdf/specupdate/297738.pdf
>        January 2002 version 017 (Nothing against the fix in the
>        specification update was found.)
>
> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
> Reviewed-by: James Hogan <james.hogan@imgtec.com>
> ---
>  drivers/pci/quirks.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index f6c31fa..3e7e489 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -414,9 +414,9 @@ static void piix4_mem_quirk(struct pci_dev *dev, const char *name, unsigned int
>         pci_read_config_dword(dev, port, &devres);
>         if ((devres & enable) != enable)
>                 return;
> -       base = devres & 0xffff0000;
> -       mask = (devres & 0x3f) << 16;
> -       size = 128 << 16;
> +       base = devres & 0xffff8000;
> +       mask = (devres & 0x7f) << 15;
> +       size = 128 << 15;
>         for (;;) {
>                 unsigned bit = size >> 1;
>                 if ((bit & mask) == bit)
> --
> 1.7.1
>
>

Re: [PATCH v2 1/4] PCI/quirks: Fix PIIX4 memory base and size mask

[Bjorn Helgaas]

On Fri, Oct 4, 2013 at 9:42 PM, DengCheng Zhu <DengCheng.Zhu@imgtec.com> wrote:
>> Does this fix a user-visible problem?  If so, what does it look like
>> when the problem occurs?
>
> No, I found the problem while debugging another issue and trying to
> understand this piece of code and after looking into different versions of
> PIIX4 datasheets. But I don't think it should prevent such a fix because
> the code is straightforward and the spec is clear enough. If the existing
> encoding was intentionally made like this by the code author due to the
> inaccuracy of the spec, then it's very likely some code comments were
> placed here. There are 2 possibilities:
>
> - This fix breaks something. People should have to bisect the problem.
> - This fix is valid. Some day people need PIIX4 mem quirks and they don't
>   have to run into a possibly well-hidden issue.
>
> What do you think?

Don't worry, I'm willing to fix it even if nobody has actually
reported a problem.  It's just nice to include the symptoms if
somebody *has* reported it, so when other people see the same symptom,
they can more easily find the fix.

Bjorn

>> I assume the other patches (2-4) are cleanups that don't fix any
>> problems.
>
> Definitely, as indicated by patch titles and commit messages.
>
>
> Deng-Cheng
>
> ________________________________________
> From: Bjorn Helgaas [bhelgaas@google.com]
> Sent: Friday, October 04, 2013 7:13 PM
> To: DengCheng Zhu
> Cc: linux-pci@vger.kernel.org; James Hogan; Qais Yousef
> Subject: Re: [PATCH v2 1/4] PCI/quirks: Fix PIIX4 memory base and size mask
>
> On Fri, Oct 4, 2013 at 5:30 PM, Deng-Cheng Zhu <dengcheng.zhu@imgtec.com> wrote:
>> From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>>
>> According to Intel PIIX4 datasheet: The memory address consists of a 17-bit
>> base address (AD[31:15]) and a 7-bit mask (AD[21:15]). This provides memory
>> ranges from 32 Kbytes to 4 Mbytes in size.
>>
>> This is true for both devices 12 and 13. This patch fixes it.
>
> Does this fix a user-visible problem?  If so, what does it look like
> when the problem occurs?
>
> I assume the other patches (2-4) are cleanups that don't fix any problems.
>
>> Ref 1: www.intel.com/assets/pdf/datasheet/290562.pdf
>>        April 1997 version 001 (p131 bottom, p226 top)
>> Ref 2: www.intel.com/assets/pdf/specupdate/297738.pdf
>>        January 2002 version 017 (Nothing against the fix in the
>>        specification update was found.)
>>
>> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>> Reviewed-by: James Hogan <james.hogan@imgtec.com>
>> ---
>>  drivers/pci/quirks.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index f6c31fa..3e7e489 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -414,9 +414,9 @@ static void piix4_mem_quirk(struct pci_dev *dev, const char *name, unsigned int
>>         pci_read_config_dword(dev, port, &devres);
>>         if ((devres & enable) != enable)
>>                 return;
>> -       base = devres & 0xffff0000;
>> -       mask = (devres & 0x3f) << 16;
>> -       size = 128 << 16;
>> +       base = devres & 0xffff8000;
>> +       mask = (devres & 0x7f) << 15;
>> +       size = 128 << 15;
>>         for (;;) {
>>                 unsigned bit = size >> 1;
>>                 if ((bit & mask) == bit)
>> --
>> 1.7.1
>>
>>
>

Re: [PATCH v2 1/4] PCI/quirks: Fix PIIX4 memory base and size mask

[Deng-Cheng Zhu]

[Resend to the mailing list due to the bounceback caused by html format]

On 10/04/2013 09:37 PM, Bjorn Helgaas wrote:
> On Fri, Oct 4, 2013 at 9:42 PM, DengCheng Zhu <DengCheng.Zhu@imgtec.com> wrote:
>>> Does this fix a user-visible problem?  If so, what does it look like
>>> when the problem occurs?
>> No, I found the problem while debugging another issue and trying to
>> understand this piece of code and after looking into different versions of
>> PIIX4 datasheets. But I don't think it should prevent such a fix because
>> the code is straightforward and the spec is clear enough. If the existing
>> encoding was intentionally made like this by the code author due to the
>> inaccuracy of the spec, then it's very likely some code comments were
>> placed here. There are 2 possibilities:
>>
>> - This fix breaks something. People should have to bisect the problem.
>> - This fix is valid. Some day people need PIIX4 mem quirks and they don't
>>    have to run into a possibly well-hidden issue.
>>
>> What do you think?
> Don't worry, I'm willing to fix it even if nobody has actually
> reported a problem.  It's just nice to include the symptoms if
> somebody *has* reported it, so when other people see the same symptom,
> they can more easily find the fix.

Ah, I see. Thanks.

So far I didn't see them on any branch in kernel/git/helgaas/pci.git 
<https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/>
Is this the right place to look at? Thanks.


Deng-Cheng

Re: [PATCH v2 1/4] PCI/quirks: Fix PIIX4 memory base and size mask

[Bjorn Helgaas]

On Mon, Oct 7, 2013 at 3:14 PM, Deng-Cheng Zhu <dengcheng.zhu@imgtec.com> wrote:
> [Resend to the mailing list due to the bounceback caused by html format]
>
>
> On 10/04/2013 09:37 PM, Bjorn Helgaas wrote:
>>
>> On Fri, Oct 4, 2013 at 9:42 PM, DengCheng Zhu <DengCheng.Zhu@imgtec.com>
>> wrote:
>>>>
>>>> Does this fix a user-visible problem?  If so, what does it look like
>>>> when the problem occurs?
>>>
>>> No, I found the problem while debugging another issue and trying to
>>> understand this piece of code and after looking into different versions
>>> of
>>> PIIX4 datasheets. But I don't think it should prevent such a fix because
>>> the code is straightforward and the spec is clear enough. If the existing
>>> encoding was intentionally made like this by the code author due to the
>>> inaccuracy of the spec, then it's very likely some code comments were
>>> placed here. There are 2 possibilities:
>>>
>>> - This fix breaks something. People should have to bisect the problem.
>>> - This fix is valid. Some day people need PIIX4 mem quirks and they don't
>>>    have to run into a possibly well-hidden issue.
>>>
>>> What do you think?
>>
>> Don't worry, I'm willing to fix it even if nobody has actually
>> reported a problem.  It's just nice to include the symptoms if
>> somebody *has* reported it, so when other people see the same symptom,
>> they can more easily find the fix.
>
>
> Ah, I see. Thanks.
>
> So far I didn't see them on any branch in kernel/git/helgaas/pci.git
> <https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/>

This change definitely makes the code match the specs you mentioned.
But we don't have any problem reports, and when Linus added this code
(6693e74a "PCI: be more verbose about resource quirks), he presumably
was looking at a spec, too.  It's possible he transcribed the masks
incorrectly, but twice in one patch?

The PIIX4 is an old part, and for something like that I would
generally say "don't touch it" because the risk of breakage outweighs
a possible fix for machines nobody uses anymore.

However, I see that the PIIX4 is emulated in, e.g., qemu, so it's
still relevant.  But before I apply this, can you please research qemu
and how it uses these registers?  For example, if you can show that
qemu emulates the registers with the additional bits you add to the
masks here, then we should be able to make linux act incorrectly by
setting those bits in a qemu BIOS.

Bjorn

Re: [PATCH v2 1/4] PCI/quirks: Fix PIIX4 memory base and size mask

[Deng-Cheng Zhu]

On 10/08/2013 04:13 PM, Bjorn Helgaas wrote:
> On Mon, Oct 7, 2013 at 3:14 PM, Deng-Cheng Zhu <dengcheng.zhu@imgtec.com> wrote:
>> [Resend to the mailing list due to the bounceback caused by html format]
>>
>>
>> On 10/04/2013 09:37 PM, Bjorn Helgaas wrote:
>>> On Fri, Oct 4, 2013 at 9:42 PM, DengCheng Zhu <DengCheng.Zhu@imgtec.com>
>>> wrote:
>>>>> Does this fix a user-visible problem?  If so, what does it look like
>>>>> when the problem occurs?
>>>> No, I found the problem while debugging another issue and trying to
>>>> understand this piece of code and after looking into different versions
>>>> of
>>>> PIIX4 datasheets. But I don't think it should prevent such a fix because
>>>> the code is straightforward and the spec is clear enough. If the existing
>>>> encoding was intentionally made like this by the code author due to the
>>>> inaccuracy of the spec, then it's very likely some code comments were
>>>> placed here. There are 2 possibilities:
>>>>
>>>> - This fix breaks something. People should have to bisect the problem.
>>>> - This fix is valid. Some day people need PIIX4 mem quirks and they don't
>>>>     have to run into a possibly well-hidden issue.
>>>>
>>>> What do you think?
>>> Don't worry, I'm willing to fix it even if nobody has actually
>>> reported a problem.  It's just nice to include the symptoms if
>>> somebody *has* reported it, so when other people see the same symptom,
>>> they can more easily find the fix.
>>
>> Ah, I see. Thanks.
>>
>> So far I didn't see them on any branch in kernel/git/helgaas/pci.git
>> <https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/>
> This change definitely makes the code match the specs you mentioned.
> But we don't have any problem reports,
It's very likely that nobody really used PIIX4 ACPI devices 12 and 13. 
Maybe some day they'll be used, or maybe they can be deleted from the code. 
But if they'll be kept in the code, had better fix it IMO.

> and when Linus added this code
> (6693e74a "PCI: be more verbose about resource quirks), he presumably
> was looking at a spec, too.  It's possible he transcribed the masks
> incorrectly, but twice in one patch?
>
> The PIIX4 is an old part, and for something like that I would
> generally say "don't touch it" because the risk of breakage outweighs
> a possible fix for machines nobody uses anymore.
>
> However, I see that the PIIX4 is emulated in, e.g., qemu, so it's
> still relevant.  But before I apply this, can you please research qemu
> and how it uses these registers?  For example, if you can show that
> qemu emulates the registers with the additional bits you add to the
> masks here, then we should be able to make linux act incorrectly by
> setting those bits in a qemu BIOS.

QEMU doesn't emulate PIIX4 ACPI devices 12 & 13, so these registers are not 
used in it.


Deng-Cheng

[PATCH v2 2/4] PCI/quirks: Rename piix4_[io|mem]_quirk to add acpi

[Deng-Cheng Zhu]

From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>

Rename piix4_[io|mem]_quirk to piix4_acpi_[io|mem]_quirk because these 2
functions only deal with the function 3 (power management) of PIIX4.

Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
Reviewed-by: James Hogan <james.hogan@imgtec.com>
---
 drivers/pci/quirks.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 3e7e489..a2b66c3 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -380,7 +380,8 @@ static void quirk_ali7101_acpi(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL,	PCI_DEVICE_ID_AL_M7101,		quirk_ali7101_acpi);
 
-static void piix4_io_quirk(struct pci_dev *dev, const char *name, unsigned int port, unsigned int enable)
+static void piix4_acpi_io_quirk(struct pci_dev *dev, const char *name,
+				unsigned int port, unsigned int enable)
 {
 	u32 devres;
 	u32 mask, size, base;
@@ -406,7 +407,8 @@ static void piix4_io_quirk(struct pci_dev *dev, const char *name, unsigned int p
 	dev_info(&dev->dev, "%s PIO at %04x-%04x\n", name, base, base + size - 1);
 }
 
-static void piix4_mem_quirk(struct pci_dev *dev, const char *name, unsigned int port, unsigned int enable)
+static void piix4_acpi_mem_quirk(struct pci_dev *dev, const char *name,
+				 unsigned int port, unsigned int enable)
 {
 	u32 devres;
 	u32 mask, size, base;
@@ -447,23 +449,23 @@ static void quirk_piix4_acpi(struct pci_dev *dev)
 	/* Device resource A has enables for some of the other ones */
 	pci_read_config_dword(dev, 0x5c, &res_a);
 
-	piix4_io_quirk(dev, "PIIX4 devres B", 0x60, 3 << 21);
-	piix4_io_quirk(dev, "PIIX4 devres C", 0x64, 3 << 21);
+	piix4_acpi_io_quirk(dev, "PIIX4 devres B", 0x60, 3 << 21);
+	piix4_acpi_io_quirk(dev, "PIIX4 devres C", 0x64, 3 << 21);
 
 	/* Device resource D is just bitfields for static resources */
 
 	/* Device 12 enabled? */
 	if (res_a & (1 << 29)) {
-		piix4_io_quirk(dev, "PIIX4 devres E", 0x68, 1 << 20);
-		piix4_mem_quirk(dev, "PIIX4 devres F", 0x6c, 1 << 7);
+		piix4_acpi_io_quirk(dev, "PIIX4 devres E", 0x68, 1 << 20);
+		piix4_acpi_mem_quirk(dev, "PIIX4 devres F", 0x6c, 1 << 7);
 	}
 	/* Device 13 enabled? */
 	if (res_a & (1 << 30)) {
-		piix4_io_quirk(dev, "PIIX4 devres G", 0x70, 1 << 20);
-		piix4_mem_quirk(dev, "PIIX4 devres H", 0x74, 1 << 7);
+		piix4_acpi_io_quirk(dev, "PIIX4 devres G", 0x70, 1 << 20);
+		piix4_acpi_mem_quirk(dev, "PIIX4 devres H", 0x74, 1 << 7);
 	}
-	piix4_io_quirk(dev, "PIIX4 devres I", 0x78, 1 << 20);
-	piix4_io_quirk(dev, "PIIX4 devres J", 0x7c, 1 << 20);
+	piix4_acpi_io_quirk(dev, "PIIX4 devres I", 0x78, 1 << 20);
+	piix4_acpi_io_quirk(dev, "PIIX4 devres J", 0x7c, 1 << 20);
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_82371AB_3,	quirk_piix4_acpi);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_82443MX_3,	quirk_piix4_acpi);
-- 
1.7.1

Re: [PATCH v2 2/4] PCI/quirks: Rename piix4_[io|mem]_quirk to add acpi

[Bjorn Helgaas]

On Fri, Oct 4, 2013 at 5:30 PM, Deng-Cheng Zhu <dengcheng.zhu@imgtec.com> wrote:
> From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>
> Rename piix4_[io|mem]_quirk to piix4_acpi_[io|mem]_quirk because these 2
> functions only deal with the function 3 (power management) of PIIX4.
>
> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
> Reviewed-by: James Hogan <james.hogan@imgtec.com>

I don't really see the point in patches 2-4.  We end up with 50 more
lines of code.  Most of this is new #defines.  They do add names where
before we only had numbers, which *sounds* attractive, but it looks
like each #define is used only once, and it's in chipset-dependent
code where a careful reader has to compare every line with the spec
anyway.  Adding a #define means you have to look at the definition,
the use, and the spec.  Without the #define, you only have to look at
the code and the spec.  I don't think it's worth it.

Bjorn

> ---
>  drivers/pci/quirks.c |   22 ++++++++++++----------
>  1 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 3e7e489..a2b66c3 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -380,7 +380,8 @@ static void quirk_ali7101_acpi(struct pci_dev *dev)
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL,     PCI_DEVICE_ID_AL_M7101,         quirk_ali7101_acpi);
>
> -static void piix4_io_quirk(struct pci_dev *dev, const char *name, unsigned int port, unsigned int enable)
> +static void piix4_acpi_io_quirk(struct pci_dev *dev, const char *name,
> +                               unsigned int port, unsigned int enable)
>  {
>         u32 devres;
>         u32 mask, size, base;
> @@ -406,7 +407,8 @@ static void piix4_io_quirk(struct pci_dev *dev, const char *name, unsigned int p
>         dev_info(&dev->dev, "%s PIO at %04x-%04x\n", name, base, base + size - 1);
>  }
>
> -static void piix4_mem_quirk(struct pci_dev *dev, const char *name, unsigned int port, unsigned int enable)
> +static void piix4_acpi_mem_quirk(struct pci_dev *dev, const char *name,
> +                                unsigned int port, unsigned int enable)
>  {
>         u32 devres;
>         u32 mask, size, base;
> @@ -447,23 +449,23 @@ static void quirk_piix4_acpi(struct pci_dev *dev)
>         /* Device resource A has enables for some of the other ones */
>         pci_read_config_dword(dev, 0x5c, &res_a);
>
> -       piix4_io_quirk(dev, "PIIX4 devres B", 0x60, 3 << 21);
> -       piix4_io_quirk(dev, "PIIX4 devres C", 0x64, 3 << 21);
> +       piix4_acpi_io_quirk(dev, "PIIX4 devres B", 0x60, 3 << 21);
> +       piix4_acpi_io_quirk(dev, "PIIX4 devres C", 0x64, 3 << 21);
>
>         /* Device resource D is just bitfields for static resources */
>
>         /* Device 12 enabled? */
>         if (res_a & (1 << 29)) {
> -               piix4_io_quirk(dev, "PIIX4 devres E", 0x68, 1 << 20);
> -               piix4_mem_quirk(dev, "PIIX4 devres F", 0x6c, 1 << 7);
> +               piix4_acpi_io_quirk(dev, "PIIX4 devres E", 0x68, 1 << 20);
> +               piix4_acpi_mem_quirk(dev, "PIIX4 devres F", 0x6c, 1 << 7);
>         }
>         /* Device 13 enabled? */
>         if (res_a & (1 << 30)) {
> -               piix4_io_quirk(dev, "PIIX4 devres G", 0x70, 1 << 20);
> -               piix4_mem_quirk(dev, "PIIX4 devres H", 0x74, 1 << 7);
> +               piix4_acpi_io_quirk(dev, "PIIX4 devres G", 0x70, 1 << 20);
> +               piix4_acpi_mem_quirk(dev, "PIIX4 devres H", 0x74, 1 << 7);
>         }
> -       piix4_io_quirk(dev, "PIIX4 devres I", 0x78, 1 << 20);
> -       piix4_io_quirk(dev, "PIIX4 devres J", 0x7c, 1 << 20);
> +       piix4_acpi_io_quirk(dev, "PIIX4 devres I", 0x78, 1 << 20);
> +       piix4_acpi_io_quirk(dev, "PIIX4 devres J", 0x7c, 1 << 20);
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,  PCI_DEVICE_ID_INTEL_82371AB_3,  quirk_piix4_acpi);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,  PCI_DEVICE_ID_INTEL_82443MX_3,  quirk_piix4_acpi);
> --
> 1.7.1
>
>

Re: [PATCH v2 2/4] PCI/quirks: Rename piix4_[io|mem]_quirk to add acpi

[Deng-Cheng Zhu]

On 10/08/2013 04:14 PM, Bjorn Helgaas wrote:
> On Fri, Oct 4, 2013 at 5:30 PM, Deng-Cheng Zhu <dengcheng.zhu@imgtec.com> wrote:
>> From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>>
>> Rename piix4_[io|mem]_quirk to piix4_acpi_[io|mem]_quirk because these 2
>> functions only deal with the function 3 (power management) of PIIX4.
>>
>> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>> Reviewed-by: James Hogan <james.hogan@imgtec.com>
> I don't really see the point in patches 2-4.  We end up with 50 more
> lines of code.  Most of this is new #defines.  They do add names where
> before we only had numbers, which *sounds* attractive, but it looks
> like each #define is used only once, and it's in chipset-dependent
> code where a careful reader has to compare every line with the spec
> anyway.  Adding a #define means you have to look at the definition,
> the use, and the spec.  Without the #define, you only have to look at
> the code and the spec.  I don't think it's worth it.

Sometimes (most of the time) reader doesn't want to dig into spec since the 
code is already there and the device is working. Reader may only want to 
know what the code is doing. The macros help (otherwise one can only know 
that pci_read_config_dword is reading a config register and then the output 
will be used to OR or AND). Variable names do give some hints, but macros 
give additional info. Yes, many of them are used only once, but they help a 
bit when used. Some examples are in the same file drivers/pci/quirks.c, 
such as ICH*. And you can easily find other examples by grepping, e.g. 
drivers/thermal/armada_thermal.c.

The above is about patch #4. About #2, it is an effort to make the code 
have more sense, as explained in the commit message. It IS an improvement, 
isn't it?

About #3, do you really think reusing the same logic by putting it in a 
function is worthless?


Deng-Cheng

[PATCH v2 3/4] PCI/quirks: Extract the size detection logic of PIIX4 ACPI io and mem

[Deng-Cheng Zhu]

From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>

Put the same algorithm in piix4_acpi_[io|mem]_quirk into a dedicated
function for size detection.

Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
Reviewed-by: James Hogan <james.hogan@imgtec.com>
---
 drivers/pci/quirks.c |   30 ++++++++++++++++--------------
 1 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a2b66c3..06762a8 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -380,6 +380,20 @@ static void quirk_ali7101_acpi(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL,	PCI_DEVICE_ID_AL_M7101,		quirk_ali7101_acpi);
 
+static inline u32 piix4_acpi_quirk_size(u32 max, u32 mask)
+{
+	u32 size = max;
+
+	for (;;) {
+		unsigned bit = size >> 1;
+		if ((bit & mask) == bit)
+			break;
+		size = bit;
+	}
+
+	return size;
+}
+
 static void piix4_acpi_io_quirk(struct pci_dev *dev, const char *name,
 				unsigned int port, unsigned int enable)
 {
@@ -391,13 +405,7 @@ static void piix4_acpi_io_quirk(struct pci_dev *dev, const char *name,
 		return;
 	mask = (devres >> 16) & 15;
 	base = devres & 0xffff;
-	size = 16;
-	for (;;) {
-		unsigned bit = size >> 1;
-		if ((bit & mask) == bit)
-			break;
-		size = bit;
-	}
+	size = piix4_acpi_quirk_size(16, mask);
 	/*
 	 * For now we only print it out. Eventually we'll want to
 	 * reserve it (at least if it's in the 0x1000+ range), but
@@ -418,13 +426,7 @@ static void piix4_acpi_mem_quirk(struct pci_dev *dev, const char *name,
 		return;
 	base = devres & 0xffff8000;
 	mask = (devres & 0x7f) << 15;
-	size = 128 << 15;
-	for (;;) {
-		unsigned bit = size >> 1;
-		if ((bit & mask) == bit)
-			break;
-		size = bit;
-	}
+	size = piix4_acpi_quirk_size(128 << 15, mask);
 	/*
 	 * For now we only print it out. Eventually we'll want to
 	 * reserve it, but let's get enough confirmation reports first. 
-- 
1.7.1

[PATCH v2 4/4] PCI/quirks: Convert hard-coded values to macros for PIIX4 ACPI quirks

[Deng-Cheng Zhu]

From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>

Get rid of a bunch of magic encodings by using macros and unify the
calculations of base/mask/size of io and mem quirks. These help code
reading easier.

Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
Reviewed-by: James Hogan <james.hogan@imgtec.com>
---
 drivers/pci/quirks.c |   86 +++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 67 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 06762a8..f4ce679 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -394,6 +394,11 @@ static inline u32 piix4_acpi_quirk_size(u32 max, u32 mask)
 	return size;
 }
 
+#define PIIX4_ACPI_DEVRES_IOSIZEMASK_MASK		0xf0000
+#define PIIX4_ACPI_DEVRES_IOSIZEMASK_ADDRLINE(m)	((m) >> 16)
+#define PIIX4_ACPI_DEVRES_IOSIZE_MAX			16
+#define PIIX4_ACPI_DEVRES_IOBASEADDR_MASK		0xffff
+
 static void piix4_acpi_io_quirk(struct pci_dev *dev, const char *name,
 				unsigned int port, unsigned int enable)
 {
@@ -403,9 +408,11 @@ static void piix4_acpi_io_quirk(struct pci_dev *dev, const char *name,
 	pci_read_config_dword(dev, port, &devres);
 	if ((devres & enable) != enable)
 		return;
-	mask = (devres >> 16) & 15;
-	base = devres & 0xffff;
-	size = piix4_acpi_quirk_size(16, mask);
+
+	mask = devres & PIIX4_ACPI_DEVRES_IOSIZEMASK_MASK;
+	mask = PIIX4_ACPI_DEVRES_IOSIZEMASK_ADDRLINE(mask);
+	base = devres & PIIX4_ACPI_DEVRES_IOBASEADDR_MASK;
+	size = piix4_acpi_quirk_size(PIIX4_ACPI_DEVRES_IOSIZE_MAX, mask);
 	/*
 	 * For now we only print it out. Eventually we'll want to
 	 * reserve it (at least if it's in the 0x1000+ range), but
@@ -415,6 +422,11 @@ static void piix4_acpi_io_quirk(struct pci_dev *dev, const char *name,
 	dev_info(&dev->dev, "%s PIO at %04x-%04x\n", name, base, base + size - 1);
 }
 
+#define PIIX4_ACPI_DEVRES_MEMSIZEMASK_MASK		0x7f
+#define PIIX4_ACPI_DEVRES_MEMSIZEMASK_ADDRLINE(m)	((m) << 15)
+#define PIIX4_ACPI_DEVRES_MEMSIZE_MAX			(128 << 15)
+#define PIIX4_ACPI_DEVRES_MEMBASEADDR_MASK		0xffff8000
+
 static void piix4_acpi_mem_quirk(struct pci_dev *dev, const char *name,
 				 unsigned int port, unsigned int enable)
 {
@@ -424,9 +436,11 @@ static void piix4_acpi_mem_quirk(struct pci_dev *dev, const char *name,
 	pci_read_config_dword(dev, port, &devres);
 	if ((devres & enable) != enable)
 		return;
-	base = devres & 0xffff8000;
-	mask = (devres & 0x7f) << 15;
-	size = piix4_acpi_quirk_size(128 << 15, mask);
+
+	mask = devres & PIIX4_ACPI_DEVRES_MEMSIZEMASK_MASK;
+	mask = PIIX4_ACPI_DEVRES_MEMSIZEMASK_ADDRLINE(mask);
+	base = devres & PIIX4_ACPI_DEVRES_MEMBASEADDR_MASK;
+	size = piix4_acpi_quirk_size(PIIX4_ACPI_DEVRES_MEMSIZE_MAX, mask);
 	/*
 	 * For now we only print it out. Eventually we'll want to
 	 * reserve it, but let's get enough confirmation reports first. 
@@ -435,6 +449,30 @@ static void piix4_acpi_mem_quirk(struct pci_dev *dev, const char *name,
 	dev_info(&dev->dev, "%s MMIO at %04x-%04x\n", name, base, base + size - 1);
 }
 
+#define PIIX4_ACPI_PMBA			0x40
+#define PIIX4_ACPI_PMBA_SIZE		64
+#define PIIX4_ACPI_SMBBA		0x90
+#define PIIX4_ACPI_SMBBA_SIZE		16
+#define PIIX4_ACPI_DEVRESA		0x5c
+#define   PIIX4_ACPI_DEVRESA_EIO_EN_DEV12	(1 << 29)
+#define   PIIX4_ACPI_DEVRESA_EIO_EN_DEV13	(1 << 30)
+#define PIIX4_ACPI_DEVRESB		0x60
+#define   PIIX4_ACPI_DEVRESB_EN_DEV9		(3 << 21)
+#define PIIX4_ACPI_DEVRESC		0x64
+#define   PIIX4_ACPI_DEVRESC_EN_DEV10		(3 << 21)
+#define PIIX4_ACPI_DEVRESE		0x68
+#define   PIIX4_ACPI_DEVRESE_IO_EN_DEV12	(1 << 20)
+#define PIIX4_ACPI_DEVRESF		0x6c
+#define   PIIX4_ACPI_DEVRESF_MEM_EN_DEV12	(1 << 7)
+#define PIIX4_ACPI_DEVRESG		0x70
+#define   PIIX4_ACPI_DEVRESG_IO_EN_DEV13	(1 << 20)
+#define PIIX4_ACPI_DEVRESH		0x74
+#define   PIIX4_ACPI_DEVRESH_MEM_EN_DEV13	(1 << 7)
+#define PIIX4_ACPI_DEVRESI		0x78
+#define   PIIX4_ACPI_DEVRESI_IO_EN_GDEC0	(1 << 20)
+#define PIIX4_ACPI_DEVRESJ		0x7c
+#define   PIIX4_ACPI_DEVRESJ_IO_EN_GDEC1	(1 << 20)
+
 /*
  * PIIX4 ACPI: Two IO regions pointed to by longwords at
  *	0x40 (64 bytes of ACPI registers)
@@ -445,29 +483,39 @@ static void quirk_piix4_acpi(struct pci_dev *dev)
 {
 	u32 res_a;
 
-	quirk_io_region(dev, 0x40, 64, PCI_BRIDGE_RESOURCES, "PIIX4 ACPI");
-	quirk_io_region(dev, 0x90, 16, PCI_BRIDGE_RESOURCES+1, "PIIX4 SMB");
+	quirk_io_region(dev, PIIX4_ACPI_PMBA, PIIX4_ACPI_PMBA_SIZE,
+			PCI_BRIDGE_RESOURCES, "PIIX4 ACPI");
+	quirk_io_region(dev, PIIX4_ACPI_SMBBA, PIIX4_ACPI_SMBBA_SIZE,
+			PCI_BRIDGE_RESOURCES + 1, "PIIX4 SMB");
 
 	/* Device resource A has enables for some of the other ones */
-	pci_read_config_dword(dev, 0x5c, &res_a);
+	pci_read_config_dword(dev, PIIX4_ACPI_DEVRESA, &res_a);
 
-	piix4_acpi_io_quirk(dev, "PIIX4 devres B", 0x60, 3 << 21);
-	piix4_acpi_io_quirk(dev, "PIIX4 devres C", 0x64, 3 << 21);
+	piix4_acpi_io_quirk(dev, "PIIX4 devres B", PIIX4_ACPI_DEVRESB,
+			    PIIX4_ACPI_DEVRESB_EN_DEV9);
+	piix4_acpi_io_quirk(dev, "PIIX4 devres C", PIIX4_ACPI_DEVRESC,
+			    PIIX4_ACPI_DEVRESC_EN_DEV10);
 
 	/* Device resource D is just bitfields for static resources */
 
 	/* Device 12 enabled? */
-	if (res_a & (1 << 29)) {
-		piix4_acpi_io_quirk(dev, "PIIX4 devres E", 0x68, 1 << 20);
-		piix4_acpi_mem_quirk(dev, "PIIX4 devres F", 0x6c, 1 << 7);
+	if (res_a & PIIX4_ACPI_DEVRESA_EIO_EN_DEV12) {
+		piix4_acpi_io_quirk(dev, "PIIX4 devres E", PIIX4_ACPI_DEVRESE,
+				    PIIX4_ACPI_DEVRESE_IO_EN_DEV12);
+		piix4_acpi_mem_quirk(dev, "PIIX4 devres F", PIIX4_ACPI_DEVRESF,
+				     PIIX4_ACPI_DEVRESF_MEM_EN_DEV12);
 	}
 	/* Device 13 enabled? */
-	if (res_a & (1 << 30)) {
-		piix4_acpi_io_quirk(dev, "PIIX4 devres G", 0x70, 1 << 20);
-		piix4_acpi_mem_quirk(dev, "PIIX4 devres H", 0x74, 1 << 7);
-	}
-	piix4_acpi_io_quirk(dev, "PIIX4 devres I", 0x78, 1 << 20);
-	piix4_acpi_io_quirk(dev, "PIIX4 devres J", 0x7c, 1 << 20);
+	if (res_a & PIIX4_ACPI_DEVRESA_EIO_EN_DEV13) {
+		piix4_acpi_io_quirk(dev, "PIIX4 devres G", PIIX4_ACPI_DEVRESG,
+				    PIIX4_ACPI_DEVRESG_IO_EN_DEV13);
+		piix4_acpi_mem_quirk(dev, "PIIX4 devres H", PIIX4_ACPI_DEVRESH,
+				     PIIX4_ACPI_DEVRESH_MEM_EN_DEV13);
+	}
+	piix4_acpi_io_quirk(dev, "PIIX4 devres I", PIIX4_ACPI_DEVRESI,
+			    PIIX4_ACPI_DEVRESI_IO_EN_GDEC0);
+	piix4_acpi_io_quirk(dev, "PIIX4 devres J", PIIX4_ACPI_DEVRESJ,
+			    PIIX4_ACPI_DEVRESJ_IO_EN_GDEC1);
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_82371AB_3,	quirk_piix4_acpi);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_82443MX_3,	quirk_piix4_acpi);
-- 
1.7.1