arm64: Define HAVE_ARCH_PIO_SIZE and related symbols.

Commit Message

David Daney July 13, 2015, 9:31 p.m. UTC

From: David Daney <[e-mail]>

Needed to make pci_iomap() work.

Signed-off-by: David Daney <[e-mail]>
---
 arch/arm64/include/asm/io.h | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Will Deacon July 14, 2015, 11 a.m. UTC | #1

On Mon, Jul 13, 2015 at 10:31:36PM +0100, David Daney wrote:
> From: David Daney <[e-mail]>
> 
> Needed to make pci_iomap() work.

Care to elaborate?

AFAICT, mapping an IO bar on arm64 just gives you back a VA into our
PCI_IOBASE region and the ioreadX accessors will just call the readX
macros, so there should be no need for further port adjustment.

Will

David Daney July 14, 2015, 4:12 p.m. UTC | #2

On 07/14/2015 04:00 AM, Will Deacon wrote:
> On Mon, Jul 13, 2015 at 10:31:36PM +0100, David Daney wrote:
>> From: David Daney <[e-mail]>
>>
>> Needed to make pci_iomap() work.
>
> Care to elaborate?
>

I should have explained what I am doing here a little better.

Systems based on the Cavium ThunderX processor may have up to 8 
independent PCIe root complexes.  The I/O space on each bus occupies an 
independent physical address window.

So, in order to be able to map all of these (semi) contiguously, we need 
a lot more virtual address space than is supplied by the default values 
for all these constants.

The option I chose here was to unconditionally expand the I/O ranges for 
all arm64 systems.  If you think this breaks existing systems/drivers, I 
will have to look for other options.

David Daney


> AFAICT, mapping an IO bar on arm64 just gives you back a VA into our
> PCI_IOBASE region and the ioreadX accessors will just call the readX
> macros, so there should be no need for further port adjustment.
>
> Will
>

Will Deacon July 14, 2015, 4:29 p.m. UTC | #3

On Tue, Jul 14, 2015 at 05:12:57PM +0100, David Daney wrote:
> On 07/14/2015 04:00 AM, Will Deacon wrote:
> > On Mon, Jul 13, 2015 at 10:31:36PM +0100, David Daney wrote:
> >> From: David Daney <[e-mail]>
> >>
> >> Needed to make pci_iomap() work.
> >
> > Care to elaborate?
> >
> 
> I should have explained what I am doing here a little better.

Yeah, thanks.

> Systems based on the Cavium ThunderX processor may have up to 8 
> independent PCIe root complexes.  The I/O space on each bus occupies an 
> independent physical address window.

Hmm, so do you have 64k of I/O space per-bus? That gives 8x256x64k = 128M
IIUC, so not sure what your 32MB is for.

> So, in order to be able to map all of these (semi) contiguously, we need 
> a lot more virtual address space than is supplied by the default values 
> for all these constants.
> 
> The option I chose here was to unconditionally expand the I/O ranges for 
> all arm64 systems.  If you think this breaks existing systems/drivers, I 
> will have to look for other options.

Hmm, but pci_iomap winds up calling __pci_ioport_map, which expands to
ioport_map which just does:

	return PCI_IOBASE + (port & IO_SPACE_LIMIT);

so I'm struggling to see what your patch achieves.

Will

David Daney July 14, 2015, 4:58 p.m. UTC | #4

On 07/14/2015 09:29 AM, Will Deacon wrote:
> On Tue, Jul 14, 2015 at 05:12:57PM +0100, David Daney wrote:
>> On 07/14/2015 04:00 AM, Will Deacon wrote:
>>> On Mon, Jul 13, 2015 at 10:31:36PM +0100, David Daney wrote:
>>>> From: David Daney <[e-mail]>
>>>>
>>>> Needed to make pci_iomap() work.
>>>
>>> Care to elaborate?
>>>
>>
>> I should have explained what I am doing here a little better.
>
> Yeah, thanks.
>
>> Systems based on the Cavium ThunderX processor may have up to 8
>> independent PCIe root complexes.  The I/O space on each bus occupies an
>> independent physical address window.
>
> Hmm, so do you have 64k of I/O space per-bus? That gives 8x256x64k = 128M
> IIUC, so not sure what your 32MB is for.

I don't understand where your 256 came from there.

Actually, my current implementation has 1M per bus(which is overkill). 
For 8 buses I need 8M, which fits within the PCI_IO_SIZE...

>
>> So, in order to be able to map all of these (semi) contiguously, we need
>> a lot more virtual address space than is supplied by the default values
>> for all these constants.
>>
>> The option I chose here was to unconditionally expand the I/O ranges for
>> all arm64 systems.  If you think this breaks existing systems/drivers, I
>> will have to look for other options.
>
> Hmm, but pci_iomap winds up calling __pci_ioport_map, which expands to
> ioport_map which just does:
>
> 	return PCI_IOBASE + (port & IO_SPACE_LIMIT);
>
> so I'm struggling to see what your patch achieves.

Here is ioport_map (from lib/iomap.c):


void __iomem *ioport_map(unsigned long port, unsigned int nr)
{
	if (port > PIO_MASK)
		return NULL;
	return (void __iomem *) (unsigned long) (port + PIO_OFFSET);
}

With the default value of PIO_MASK (64K), I cannot map any I/O ports on 
my PCIe RC 1..7

The values I supplied in my patch may be sub-optimal, but I think 
something is needed.  I will look into this in a little more detail today.

Thanks,
David Daney


>
> Will
>

Will Deacon July 14, 2015, 5:04 p.m. UTC | #5

Hi David,

On Tue, Jul 14, 2015 at 05:58:20PM +0100, David Daney wrote:
> On 07/14/2015 09:29 AM, Will Deacon wrote:
> > On Tue, Jul 14, 2015 at 05:12:57PM +0100, David Daney wrote:
> >> Systems based on the Cavium ThunderX processor may have up to 8
> >> independent PCIe root complexes.  The I/O space on each bus occupies an
> >> independent physical address window.
> >
> > Hmm, so do you have 64k of I/O space per-bus? That gives 8x256x64k = 128M
> > IIUC, so not sure what your 32MB is for.
> 
> I don't understand where your 256 came from there.

Sorry, sounds like we're mixing up buses and root complexes. Which is it?

> Actually, my current implementation has 1M per bus(which is overkill). 
> For 8 buses I need 8M, which fits within the PCI_IO_SIZE...

So there's no problem?

> >> So, in order to be able to map all of these (semi) contiguously, we need
> >> a lot more virtual address space than is supplied by the default values
> >> for all these constants.
> >>
> >> The option I chose here was to unconditionally expand the I/O ranges for
> >> all arm64 systems.  If you think this breaks existing systems/drivers, I
> >> will have to look for other options.
> >
> > Hmm, but pci_iomap winds up calling __pci_ioport_map, which expands to
> > ioport_map which just does:
> >
> > 	return PCI_IOBASE + (port & IO_SPACE_LIMIT);
> >
> > so I'm struggling to see what your patch achieves.
> 
> Here is ioport_map (from lib/iomap.c):
> 
> 
> void __iomem *ioport_map(unsigned long port, unsigned int nr)
> {
> 	if (port > PIO_MASK)
> 		return NULL;
> 	return (void __iomem *) (unsigned long) (port + PIO_OFFSET);
> }

We only get this definition if CONFIG_GENERIC_IOMAP=y. Why is that
selected?

Will

David Daney July 14, 2015, 5:54 p.m. UTC | #6

On 07/14/2015 10:04 AM, Will Deacon wrote:
> Hi David,
>
> On Tue, Jul 14, 2015 at 05:58:20PM +0100, David Daney wrote:
[...]
> We only get this definition if CONFIG_GENERIC_IOMAP=y. Why is that
> selected?
>

OK, Good question.

The original patch was against 3.18, but  commit 09a5723983e (arm64: Use 
include/asm-generic/io.h) changed some of this, so it may need 
reconsideration.

Let me reevaluate this patch.

Thanks,
David Daney

Patch

diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 540f7c0..8ef78d5 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -149,6 +149,11 @@  static inline u64 __raw_readq(const volatile void __iomem *addr)
 #define IO_SPACE_LIMIT		(PCI_IO_SIZE - 1)
 #define PCI_IOBASE		((void __iomem *)PCI_IO_START)
 
+#define HAVE_ARCH_PIO_SIZE	1
+#define PIO_RESERVED		SZ_32M
+#define PIO_OFFSET		0
+#define PIO_MASK		(PIO_RESERVED - 1)
+
 /*
  * String version of I/O memory access operations.
  */