mach/io.h cleanup and removal question

mach/io.h cleanup and removal question

[Andrew Lunn]

Hi Rob

Your patchset for mach/io.h cleanup and remove, causes problems on
Orion5x.

mach/io.h for orion5x had a:

#define IO_SPACE_LIMIT		0xffffffff

which got removed. This results in a panic at boot:

        /*
         * IORESOURCE_IO
         */
        sys->io_offset = 0;
        res[0].name = "PCIe I/O Space";
        res[0].flags = IORESOURCE_IO;
        res[0].start = ORION5X_PCIE_IO_BUS_BASE;
        res[0].end = res[0].start + ORION5X_PCIE_IO_SIZE - 1;
        if (request_resource(&ioport_resource, &res[0]))
                panic("Request PCIe IO resource failed\n");

ORION5X_PCIE_IO_SIZE is 1MB, so the allocation fails because of the
64K default.

arch/arm/include/asm/io.h has the comment:

/*
 * This is the limit of PC card/PCI/ISA IO space, which is by default
 * 64K if we have PC card, PCI or ISA support.  Otherwise, default to
 * zero to prevent ISA/PCI drivers claiming IO space (and potentially
 * oopsing.)
 *
 * Only set this larger if you really need inb() et.al. to operate over
 * a larger address space.  Note that SOC_COMMON ioremaps each sockets
 * IO space area, and so inb() et.al. must be defined to operate as per
 * readb() et.al. on such platforms.
 */

Now, i know nothing about how PCI works... So i have a question:

Which is better, put back parts of io.h so allowing the 1MB
request_resource, or reduce ORION5X_PCIE_IO_SIZE to 64KB, since from
the comment it is unlikely an PCI card needs more than 64KB?

    Thanks
	Andrew

mach/io.h cleanup and removal question

[Russell King - ARM Linux]

On Fri, Jun 08, 2012 at 12:20:40PM +0200, Andrew Lunn wrote:
> Your patchset for mach/io.h cleanup and remove, causes problems on
> Orion5x.
> 
> mach/io.h for orion5x had a:
> 
> #define IO_SPACE_LIMIT		0xffffffff
> 
> which got removed. This results in a panic at boot:
> 
>         /*
>          * IORESOURCE_IO
>          */
>         sys->io_offset = 0;
>         res[0].name = "PCIe I/O Space";
>         res[0].flags = IORESOURCE_IO;
>         res[0].start = ORION5X_PCIE_IO_BUS_BASE;
>         res[0].end = res[0].start + ORION5X_PCIE_IO_SIZE - 1;
>         if (request_resource(&ioport_resource, &res[0]))
>                 panic("Request PCIe IO resource failed\n");
> 
> ORION5X_PCIE_IO_SIZE is 1MB, so the allocation fails because of the
> 64K default.
> 
> arch/arm/include/asm/io.h has the comment:
> 
> /*
>  * This is the limit of PC card/PCI/ISA IO space, which is by default
>  * 64K if we have PC card, PCI or ISA support.  Otherwise, default to
>  * zero to prevent ISA/PCI drivers claiming IO space (and potentially
>  * oopsing.)
>  *
>  * Only set this larger if you really need inb() et.al. to operate over
>  * a larger address space.  Note that SOC_COMMON ioremaps each sockets
>  * IO space area, and so inb() et.al. must be defined to operate as per
>  * readb() et.al. on such platforms.
>  */
> 
> Now, i know nothing about how PCI works... So i have a question:
> 
> Which is better, put back parts of io.h so allowing the 1MB
> request_resource, or reduce ORION5X_PCIE_IO_SIZE to 64KB, since from
> the comment it is unlikely an PCI card needs more than 64KB?

Well, the comment about SOC_COMMON doesn't apply as you're not using that.

Probably shrinking ORION5X_PCIE_IO_SIZE to 64K, unless we really have a
requirement to support a larger IO space.

mach/io.h cleanup and removal question

[Rob Herring]

On 06/08/2012 06:43 AM, Russell King - ARM Linux wrote:
> On Fri, Jun 08, 2012 at 12:20:40PM +0200, Andrew Lunn wrote:
>> Your patchset for mach/io.h cleanup and remove, causes problems on
>> Orion5x.
>>
>> mach/io.h for orion5x had a:
>>
>> #define IO_SPACE_LIMIT		0xffffffff
>>
>> which got removed. This results in a panic at boot:
>>
>>         /*
>>          * IORESOURCE_IO
>>          */
>>         sys->io_offset = 0;
>>         res[0].name = "PCIe I/O Space";
>>         res[0].flags = IORESOURCE_IO;
>>         res[0].start = ORION5X_PCIE_IO_BUS_BASE;
>>         res[0].end = res[0].start + ORION5X_PCIE_IO_SIZE - 1;
>>         if (request_resource(&ioport_resource, &res[0]))
>>                 panic("Request PCIe IO resource failed\n");
>>
>> ORION5X_PCIE_IO_SIZE is 1MB, so the allocation fails because of the
>> 64K default.
>>
>> arch/arm/include/asm/io.h has the comment:
>>
>> /*
>>  * This is the limit of PC card/PCI/ISA IO space, which is by default
>>  * 64K if we have PC card, PCI or ISA support.  Otherwise, default to
>>  * zero to prevent ISA/PCI drivers claiming IO space (and potentially
>>  * oopsing.)
>>  *
>>  * Only set this larger if you really need inb() et.al. to operate over
>>  * a larger address space.  Note that SOC_COMMON ioremaps each sockets
>>  * IO space area, and so inb() et.al. must be defined to operate as per
>>  * readb() et.al. on such platforms.
>>  */
>>
>> Now, i know nothing about how PCI works... So i have a question:
>>
>> Which is better, put back parts of io.h so allowing the 1MB
>> request_resource, or reduce ORION5X_PCIE_IO_SIZE to 64KB, since from
>> the comment it is unlikely an PCI card needs more than 64KB?
> 
> Well, the comment about SOC_COMMON doesn't apply as you're not using that.
> 
> Probably shrinking ORION5X_PCIE_IO_SIZE to 64K, unless we really have a
> requirement to support a larger IO space.

Agreed. The next step is moving all i/o space to a fixed virtual address
and for this we want to make i/o windows 64K.

However, I think you may also have a problem with
ORION5X_PCIE_IO_BUS_BASE and the resource start. The i/o resource start
should really be 0 and then the __io() macro should add the virtual i/o
base address. However, Russell has mentioned previously that some chips
may not work correctly with i/o space at 0 (PCI bus addresses, not host).

Do you have cards with i/o that you can test or know what devices are
used with this platform?

Rob

mach/io.h cleanup and removal question

[Andrew Lunn]

> > Probably shrinking ORION5X_PCIE_IO_SIZE to 64K, unless we really have a
> > requirement to support a larger IO space.
> 
> Agreed. The next step is moving all i/o space to a fixed virtual address
> and for this we want to make i/o windows 64K.

O.K. I will submit a patch for this, so Orion5x at least boots. Maybe
PCI will not work, but booting is a step forward.

> However, I think you may also have a problem with
> ORION5X_PCIE_IO_BUS_BASE and the resource start. The i/o resource start
> should really be 0 and then the __io() macro should add the virtual i/o
> base address. However, Russell has mentioned previously that some chips
> may not work correctly with i/o space at 0 (PCI bus addresses, not host).
> 
> Do you have cards with i/o that you can test or know what devices are
> used with this platform?

Humm, not sure. I have a Marvell Orion5x NAS reference design. It has
a Mini-PCI slot on the bottom. So if i can find a card, i can test. I
know i have some WiFi cards in my junk box, but i've no idea if these
have i/o.

     Andrew

mach/io.h cleanup and removal question

[Andrew Lunn]

On Fri, Jun 08, 2012 at 09:10:17AM -0500, Rob Herring wrote:
> On 06/08/2012 06:43 AM, Russell King - ARM Linux wrote:
> > On Fri, Jun 08, 2012 at 12:20:40PM +0200, Andrew Lunn wrote:
> >> Your patchset for mach/io.h cleanup and remove, causes problems on
> >> Orion5x.
> >>
> >> mach/io.h for orion5x had a:
> >>
> >> #define IO_SPACE_LIMIT		0xffffffff
> >>
> >> which got removed. This results in a panic at boot:
> >>
> >>         /*
> >>          * IORESOURCE_IO
> >>          */
> >>         sys->io_offset = 0;
> >>         res[0].name = "PCIe I/O Space";
> >>         res[0].flags = IORESOURCE_IO;
> >>         res[0].start = ORION5X_PCIE_IO_BUS_BASE;
> >>         res[0].end = res[0].start + ORION5X_PCIE_IO_SIZE - 1;
> >>         if (request_resource(&ioport_resource, &res[0]))
> >>                 panic("Request PCIe IO resource failed\n");
> >>
> >> ORION5X_PCIE_IO_SIZE is 1MB, so the allocation fails because of the
> >> 64K default.
> >>
> >> arch/arm/include/asm/io.h has the comment:
> >>
> >> /*
> >>  * This is the limit of PC card/PCI/ISA IO space, which is by default
> >>  * 64K if we have PC card, PCI or ISA support.  Otherwise, default to
> >>  * zero to prevent ISA/PCI drivers claiming IO space (and potentially
> >>  * oopsing.)
> >>  *
> >>  * Only set this larger if you really need inb() et.al. to operate over
> >>  * a larger address space.  Note that SOC_COMMON ioremaps each sockets
> >>  * IO space area, and so inb() et.al. must be defined to operate as per
> >>  * readb() et.al. on such platforms.
> >>  */
> >>
> >> Now, i know nothing about how PCI works... So i have a question:
> >>
> >> Which is better, put back parts of io.h so allowing the 1MB
> >> request_resource, or reduce ORION5X_PCIE_IO_SIZE to 64KB, since from
> >> the comment it is unlikely an PCI card needs more than 64KB?
> > 
> > Well, the comment about SOC_COMMON doesn't apply as you're not using that.
> > 
> > Probably shrinking ORION5X_PCIE_IO_SIZE to 64K, unless we really have a
> > requirement to support a larger IO space.
> 
> Agreed. The next step is moving all i/o space to a fixed virtual address
> and for this we want to make i/o windows 64K.

Hi Rob

Reducing the window size to 64K stops the panic at boot, as expected.

> 
> However, I think you may also have a problem with
> ORION5X_PCIE_IO_BUS_BASE and the resource start. The i/o resource start
> should really be 0 and then the __io() macro should add the virtual i/o
> base address. However, Russell has mentioned previously that some chips
> may not work correctly with i/o space at 0 (PCI bus addresses, not host).
> 
> Do you have cards with i/o that you can test or know what devices are
> used with this platform?

I don't have any cards. I've also no idea what cards are used with
these devices. I guess they are mostly WiFi cards, since Orion5x is
normally used in NAS boxes, and there is one box supported which is an
WiFi AP.

Since the panic kills all orion5x systems, i want to at least fix
that. So i will post the patch to reduce the window size to 64K.  I
guess for the moment we have to leave PCI potentially broken, and see
if anybody complains.

Is this O.K?

   Thanks
	Andrew

mach/io.h cleanup and removal question

[Nicolas Pitre]

On Mon, 18 Jun 2012, Andrew Lunn wrote:
> On Fri, Jun 08, 2012 at 09:10:17AM -0500, Rob Herring wrote:
> > Do you have cards with i/o that you can test or know what devices are
> > used with this platform?
> 
> I don't have any cards. I've also no idea what cards are used with
> these devices. I guess they are mostly WiFi cards, since Orion5x is
> normally used in NAS boxes, and there is one box supported which is an
> WiFi AP.

Those are also mini PCI-E cards, and I would be very surprised if one 
could ever find such a card making use of the PCI I/O space.  So this 
might prove very difficult to actually test this support, and I can't 
remember if the current code was ever confirmed to work in the first 
place because of no available cards.  So the best thing that can be done 
is to make sure it looks right from a theoretical point of view.


Nicolas

mach/io.h cleanup and removal question

[Russell King - ARM Linux]

On Fri, Jun 08, 2012 at 09:10:17AM -0500, Rob Herring wrote:
> Agreed. The next step is moving all i/o space to a fixed virtual address
> and for this we want to make i/o windows 64K.
> 
> However, I think you may also have a problem with
> ORION5X_PCIE_IO_BUS_BASE and the resource start. The i/o resource start
> should really be 0 and then the __io() macro should add the virtual i/o
> base address. However, Russell has mentioned previously that some chips
> may not work correctly with i/o space at 0 (PCI bus addresses, not host).
> 
> Do you have cards with i/o that you can test or know what devices are
> used with this platform?

I/O cards themselves should not be a problem - remember, they're designed
and tested in x86 PCs where IO space is only at 0-0xffff inclusive.  Many
of these cards will only decode up to 16 bits of IO address anyway, and
they'll ignore the higher order PCI address bits (because... x86 PC,
what's the point of any more.)

The problem seems to be any PCI bridges, whether they're P2P or the host
to PCI bridge, and whether it can do address translation.  In other words,
do they just forward the host bus physical address for IO accesses -
which means any PCI card doing a full 32-bit decode on their IO BARs is
not going to work when programmed in the 0-0xffff range.

It's all rather horrible, and it can _only_ be changed and checked by
people who have access to the physical boards _and_ at least one PCI
card known to do the full 32-bit IO decode, or who are adept enough
with analysers or scopes be able to analyse the PCI bus.