[PATCH, RFC] PCI: deal with device incorrectly reporting I/O decoding being enabled

[PATCH, RFC] PCI: deal with device incorrectly reporting I/O decoding being enabled

[Lennert Buytenhek]

At least the Lava Quattro quad-port 16550A card can incorrectly report
I/O decoding being enabled while it is in fact not, which means that
the check in pcibios_enable_device() to see whether the new command
word that we're intending to write into the device is different from
the old current can trigger inadvertently, resulting in the write to
enable I/O decoding never being done, and I/O decoding never being
enabled.

Work around this by doing the write unconditionally (while still
only doing the printk if the new word is different from the old one,
to avoid dmesg spam).

Signed-off-by: Lennert Buytenhek <buytenh@wantstofly.org>
--
This is probably at least slightly controversial.  Any thoughts?

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 8096819..cf170b9 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -662,11 +662,17 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
 	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
 		cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
 
-	if (cmd != old_cmd) {
+	/*
+	 * Some devices (e.g. the Lava Quattro quad-port 16550A card)
+	 * can incorrectly report I/O decoding being enabled while it
+	 * is in fact not, so unconditionally perform the config space
+	 * write.
+	 */
+	if (cmd != old_cmd)
 		printk("PCI: enabling device %s (%04x -> %04x)\n",
 		       pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
+	pci_write_config_word(dev, PCI_COMMAND, cmd);
+
 	return 0;
 }
 
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 706f82d..96fe13d 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -322,10 +322,16 @@ int pci_enable_resources(struct pci_dev *dev, int mask)
 			cmd |= PCI_COMMAND_MEMORY;
 	}
 
-	if (cmd != old_cmd) {
+	/*
+	 * Some devices (e.g. the Lava Quattro quad-port 16550A card)
+	 * can incorrectly report I/O decoding being enabled while it
+	 * is in fact not, so unconditionally perform the config space
+	 * write.
+	 */
+	if (cmd != old_cmd)
 		dev_info(&dev->dev, "enabling device (%04x -> %04x)\n",
 			 old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
+	pci_write_config_word(dev, PCI_COMMAND, cmd);
+
 	return 0;
 }

--

[PATCH,RFC] PCI: deal with device incorrectly reporting I/O decoding being enabled

[Matthew Wilcox]

On Sat, Nov 07, 2009 at 02:56:15PM +0100, Lennert Buytenhek wrote:
> At least the Lava Quattro quad-port 16550A card can incorrectly report
> I/O decoding being enabled while it is in fact not, which means that
> the check in pcibios_enable_device() to see whether the new command
> word that we're intending to write into the device is different from
> the old current can trigger inadvertently, resulting in the write to
> enable I/O decoding never being done, and I/O decoding never being
> enabled.
> 
> Work around this by doing the write unconditionally (while still
> only doing the printk if the new word is different from the old one,
> to avoid dmesg spam).
> 
> Signed-off-by: Lennert Buytenhek <buytenh@wantstofly.org>
> --
> This is probably at least slightly controversial.  Any thoughts?

I'm OK with this ... it might provoke bugs in some _other_ piece of hardware,
but that seems pretty unlikely.

Reviewed-by: Matthew Wilcox <willy@linux.intel.com>

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

[PATCH,RFC] PCI: deal with device incorrectly reporting I/O decoding being enabled

[Russell King - ARM Linux]

On Sat, Nov 07, 2009 at 08:03:05AM -0700, Matthew Wilcox wrote:
> I'm OK with this ... it might provoke bugs in some _other_ piece of hardware,
> but that seems pretty unlikely.

Please remember that some ARM hardware only has the capability to read
and write full 32-bit quantities of configuration space, which means
writing the control register can result in the status register being
written as well.

That shouldn't be a problem, but is merely something else to consider
which isn't obvious.

[PATCH,RFC] PCI: deal with device incorrectly reporting I/O decoding being enabled

[Matthew Wilcox]

On Sun, Nov 08, 2009 at 09:04:51AM +0000, Russell King - ARM Linux wrote:
> On Sat, Nov 07, 2009 at 08:03:05AM -0700, Matthew Wilcox wrote:
> > I'm OK with this ... it might provoke bugs in some _other_ piece of hardware,
> > but that seems pretty unlikely.
> 
> Please remember that some ARM hardware only has the capability to read
> and write full 32-bit quantities of configuration space, which means
> writing the control register can result in the status register being
> written as well.
> 
> That shouldn't be a problem, but is merely something else to consider
> which isn't obvious.

Wow, that's horrid.  You must have to special case each config register
that's accessed in a 16-bit way, since some bits are 'preserve' and others
are 'write 1 to clear'.  What a nightmare.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

[PATCH,RFC] PCI: deal with device incorrectly reporting I/O decoding being enabled

[Russell King - ARM Linux]

On Sun, Nov 08, 2009 at 08:23:10AM -0700, Matthew Wilcox wrote:
> On Sun, Nov 08, 2009 at 09:04:51AM +0000, Russell King - ARM Linux wrote:
> > On Sat, Nov 07, 2009 at 08:03:05AM -0700, Matthew Wilcox wrote:
> > > I'm OK with this ... it might provoke bugs in some _other_ piece of hardware,
> > > but that seems pretty unlikely.
> > 
> > Please remember that some ARM hardware only has the capability to read
> > and write full 32-bit quantities of configuration space, which means
> > writing the control register can result in the status register being
> > written as well.
> > 
> > That shouldn't be a problem, but is merely something else to consider
> > which isn't obvious.
> 
> Wow, that's horrid.  You must have to special case each config register
> that's accessed in a 16-bit way, since some bits are 'preserve' and others
> are 'write 1 to clear'.  What a nightmare.

Yes, it's horrid, and it can be found on all IOP ARM stuff, eg:

                val = iop3xx_read(addr);
                if (iop3xx_pci_status())
                        return PCIBIOS_SUCCESSFUL;

                where = (where & 3) * 8;

                if (size == 1)
                        val &= ~(0xff << where);
                else
                        val &= ~(0xffff << where);

                *IOP3XX_OCCDR = val | value << where;

This covers IOP32x, IOP33x, and there's similar code for IOP13xx.

This does mean that if we make this change, we may do more accidental
writes to the status register on such hardware.  Whether that matters
in reality isn't something I can answer - I suspect we can get away
with this without causing problems.

CC'ing Dan for his info: the proposed change@the start of this thread
is to make the write to the command register in pcibios_enable_device()
unconditional.  This will guarantee that IOP platforms to copy-back the
status register on to itself on pci_enable_device(), thereby causing
"write 1 to clear" status bits to be cleared.

[PATCH,RFC] PCI: deal with device incorrectly reporting I/O decoding being enabled

[Matthew Wilcox]

On Sun, Nov 08, 2009 at 03:41:03PM +0000, Russell King - ARM Linux wrote:
> On Sun, Nov 08, 2009 at 08:23:10AM -0700, Matthew Wilcox wrote:
> > On Sun, Nov 08, 2009 at 09:04:51AM +0000, Russell King - ARM Linux wrote:
> > > Please remember that some ARM hardware only has the capability to read
> > > and write full 32-bit quantities of configuration space, which means
> > > writing the control register can result in the status register being
> > > written as well.
> > > 
> > > That shouldn't be a problem, but is merely something else to consider
> > > which isn't obvious.
> > 
> > Wow, that's horrid.  You must have to special case each config register
> > that's accessed in a 16-bit way, since some bits are 'preserve' and others
> > are 'write 1 to clear'.  What a nightmare.
> 
> Yes, it's horrid, and it can be found on all IOP ARM stuff, eg:
> 
>                 val = iop3xx_read(addr);
>                 if (iop3xx_pci_status())
>                         return PCIBIOS_SUCCESSFUL;
> 
>                 where = (where & 3) * 8;
> 
>                 if (size == 1)
>                         val &= ~(0xff << where);
>                 else
>                         val &= ~(0xffff << where);
> 
>                 *IOP3XX_OCCDR = val | value << where;
> 
> This covers IOP32x, IOP33x, and there's similar code for IOP13xx.
> 
> This does mean that if we make this change, we may do more accidental
> writes to the status register on such hardware.  Whether that matters
> in reality isn't something I can answer - I suspect we can get away
> with this without causing problems.

You can fix this for the status register like so:

                val = iop3xx_read(addr);
                if (iop3xx_pci_status())
                        return PCIBIOS_SUCCESSFUL;

+		/* Don't clear the RW1C bits in the Status register */
+		if (where == PCI_COMMAND)
+			val &= 0xffff;

                where = (where & 3) * 8;

                if (size == 1)
                        val &= ~(0xff << where);
                else
                        val &= ~(0xffff << where);

                *IOP3XX_OCCDR = val | value << where;

Unfortunately, this status register is not the only one with RW1C, RW1CS
or RsvdZ bits.  There's the Secondary Status Register (for bridges), a
dozen PCIe Status Registers, the Dynamic Power Allocation Status Register,
the Power Management Control/Status Register ... SHPC defines a RW1C
bit in a register, not sure if we'll hit that case.  I bet there's others.

I can only hope that in general, one does not find many PCI devices
attached to an IOP3xx.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

[PATCH,RFC] PCI: deal with device incorrectly reporting I/O decoding being enabled

[Dan Williams]

On Sun, Nov 8, 2009 at 8:41 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Nov 08, 2009 at 08:23:10AM -0700, Matthew Wilcox wrote:
>> On Sun, Nov 08, 2009 at 09:04:51AM +0000, Russell King - ARM Linux wrote:
>> > On Sat, Nov 07, 2009 at 08:03:05AM -0700, Matthew Wilcox wrote:
>> > > I'm OK with this ... it might provoke bugs in some _other_ piece of hardware,
>> > > but that seems pretty unlikely.
>> >
>> > Please remember that some ARM hardware only has the capability to read
>> > and write full 32-bit quantities of configuration space, which means
>> > writing the control register can result in the status register being
>> > written as well.
>> >
>> > That shouldn't be a problem, but is merely something else to consider
>> > which isn't obvious.
>>
>> Wow, that's horrid. ?You must have to special case each config register
>> that's accessed in a 16-bit way, since some bits are 'preserve' and others
>> are 'write 1 to clear'. ?What a nightmare.
>
> Yes, it's horrid, and it can be found on all IOP ARM stuff, eg:
>
> ? ? ? ? ? ? ? ?val = iop3xx_read(addr);
> ? ? ? ? ? ? ? ?if (iop3xx_pci_status())
> ? ? ? ? ? ? ? ? ? ? ? ?return PCIBIOS_SUCCESSFUL;
>
> ? ? ? ? ? ? ? ?where = (where & 3) * 8;
>
> ? ? ? ? ? ? ? ?if (size == 1)
> ? ? ? ? ? ? ? ? ? ? ? ?val &= ~(0xff << where);
> ? ? ? ? ? ? ? ?else
> ? ? ? ? ? ? ? ? ? ? ? ?val &= ~(0xffff << where);
>
> ? ? ? ? ? ? ? ?*IOP3XX_OCCDR = val | value << where;
>
> This covers IOP32x, IOP33x, and there's similar code for IOP13xx.
>
> This does mean that if we make this change, we may do more accidental
> writes to the status register on such hardware. ?Whether that matters
> in reality isn't something I can answer - I suspect we can get away
> with this without causing problems.
>
> CC'ing Dan for his info: the proposed change at the start of this thread
> is to make the write to the command register in pcibios_enable_device()
> unconditional. ?This will guarantee that IOP platforms to copy-back the
> status register on to itself on pci_enable_device(), thereby causing
> "write 1 to clear" status bits to be cleared.
>

Thanks for the CC.  I am hoping that this is just an initial
misreading of the IOP specification that was blindly forward ported
from the iop32x days.  In my interpretation it says that accesses may
not *cross* a dword boundary, it says nothing about sub-dword.


>From bogus@does.not.exist.com  Fri Nov  6 13:01:15 2009
From: bogus@does.not.exist.com ()
Date: Fri, 06 Nov 2009 18:01:15 -0000
Subject: No subject
Message-ID: <mailman.4.1257813337.2170.linux-arm-kernel@lists.infradead.org>

"Configuration cycles are non-burst and restricted to a single 32-bit
word cycle"
...
"The user should designate the memory region containing the OCCDR as
non-cacheable and non-bufferable from the Intel XScale=AE core.  This
guarantees that all load/stores to the OCCDR is only of DWORD
quantities. In event the user inadvertently issues a read to the OCCDR
that crosses a DWORD address boundary, the ATU target aborts the
transaction. All writes is terminated with a Single-Phase-Disconnect
and only bytes 3:0 is relevant."

I believe sub-dword access were once verified to work, but I no longer
have access to those test results.  However, the following at least
boots on my iop13xx, so I'll look into this a bit more.

Thanks,
Dan

diff --git a/arch/arm/mach-iop13xx/pci.c b/arch/arm/mach-iop13xx/pci.c
index 4873f26..1b6a65a 100644
--- a/arch/arm/mach-iop13xx/pci.c
+++ b/arch/arm/mach-iop13xx/pci.c
@@ -190,7 +190,7 @@ static u32 iop13xx_atux_cfg_address(struct pci_bus
*bus, int devfn, int where)
        else
                addr =3D bus->number << 16 | PCI_SLOT(devfn) << 11 | 1;

-       addr |=3D PCI_FUNC(devfn) << 8 | ((where & 0xff) & ~3);
+       addr |=3D PCI_FUNC(devfn) << 8 | (where & 0xff);
        addr |=3D ((where & 0xf00) >> 8) << 24; /* upper register number */

        return addr;
@@ -264,7 +264,7 @@ static u32 iop13xx_atux_read(unsigned long addr)
 {
        u32 val;

-       __asm__ __volatile__(
+       asm volatile(
                "str    %1, [%2]\n\t"
                "ldr    %0, [%3]\n\t"
                "mov    %0, %0\n\t"
@@ -281,7 +281,7 @@ static int
 iop13xx_atux_read_config(struct pci_bus *bus, unsigned int devfn, int wher=
e,
                int size, u32 *value)
 {
-       unsigned long addr =3D iop13xx_atux_cfg_address(bus, devfn, where);
+       unsigned long addr =3D iop13xx_atux_cfg_address(bus, devfn, where &=
 ~3);
        u32 val =3D iop13xx_atux_read(addr) >> ((where & 3) * 8);

        if (iop13xx_atux_pci_status(1) || is_atux_occdr_error()) {
@@ -300,25 +300,14 @@ iop13xx_atux_write_config(struct pci_bus *bus,
unsigned int devfn, int where,
                int size, u32 value)
 {
        unsigned long addr =3D iop13xx_atux_cfg_address(bus, devfn, where);
-       u32 val;
-
-       if (size !=3D 4) {
-               val =3D iop13xx_atux_read(addr);
-               if (!iop13xx_atux_pci_status(1) =3D=3D 0)
-                       return PCIBIOS_SUCCESSFUL;
-
-               where =3D (where & 3) * 8;

-               if (size =3D=3D 1)
-                       val &=3D ~(0xff << where);
-               else
-                       val &=3D ~(0xffff << where);
-
-               __raw_writel(val | value << where, IOP13XX_ATUX_OCCDR);
-       } else {
-               __raw_writel(addr, IOP13XX_ATUX_OCCAR);
+       __raw_writel(addr, IOP13XX_ATUX_OCCAR);
+       if (size =3D=3D 1)
+               __raw_writeb(value, IOP13XX_ATUX_OCCDR);
+       else if (size =3D=3D 2)
+               __raw_writew(value, IOP13XX_ATUX_OCCDR);
+       else
                __raw_writel(value, IOP13XX_ATUX_OCCDR);
-       }

        return PCIBIOS_SUCCESSFUL;
 }