[parisc-linux-cvs] [PATCH] PCI MMIO mapping

[Bjorn Helgaas]

Thanks for the detailed feedback!

> > @@ -884,7 +898,6 @@ iosapic_enable_irq(void *dev, int irq)
> >
> >  	/* data is initialized by fixup_irq */
> >  	ASSERT(0 < vi->vi_txn_irq);
> > -	ASSERT(0UL != vi->vi_txn_addr);
> >  	ASSERT(0UL != vi->vi_txn_data);
>
> Why was the ASSERT removed?
> I thought txn_addr will always be non-zero for PA-RISC.
> Can txn_addr be zero?

txn_addr is zero for cell 0, cpu 0 on superdome.

> Me either. Sorry. I think the comment should read:
> 	/*
> 	** Sign extend all BAR values on "legacy" platforms.
> 	** "Sprockets" PDC (Forte/Allegro) initializes everything for
> 	** for "legacy" 32-bit OS (HPUX 10.20).
> 	** Upper 32-bits of 64-bit BAR will be zero too.
> 	*/
>
> > -	lba_dev->lmmio_base = 0xffffffff00000000UL;
> > +	lba_dev->hba.mem_space_offset = 0xffffffff00000000UL;
> > +#else
> > +	lba_dev->hba.mem_space_offset = 0UL;
> >  #endif

I can parse it now, but I don't think it is quite correct yet.  On 64-bit 
"legacy" (non-PAT) boxes, we set the 32 high-order bits of lmmio_base, 
then later do "res->start |= ldev->lmmio_base".  This isn't sign-extension 
because it doesn't depend on the value of a sign bit.  Right?

> > @@ -1166,10 +1164,8 @@ lba_legacy_resources( struct hp_device *
> >  	r->name  = "LBA PCI LMMIO";
> >  	r->flags = IORESOURCE_MEM;
> >  	/* Ignore "Range Enable" bit in the BASE register */
> > -	r->start = ((long) READ_REG32(d->hpa + LBA_LMMIO_BASE)) & ~1UL;
> > -#ifdef __LP64__
> > -	r->start |= 0xffffffff00000000UL; /* sign extend f-space */
> > -#endif
> > +	r->start = PCI_HOST_ADDR(HBA_DATA(lba_dev),
> > +		((unsigned long) READ_REG32(d->hpa + LBA_LMMIO_BASE)) & ~1UL);
>
> This has me slightly nervous.
> Does this work on both C3000 and A500? (ie Sprockets and PAT PDCs)
> If so, does it work because the value is sign extended?
> Or because the READ_REG32() is cast to a 64-bit read and both PDCs
> write a 64-bit value into LMMIO_BASE?

I haven't tested on anything but Superdome yet, because I was ignorant of 
what the important test cases were.  I'll test C3000 and A500 before 
checking in.  I did not intend to be relying on any sign extension or 
side-effects of casting.

As I understand it, the current behavior is:

	32-bit boxes:
		host address == PCI bus address always

	64-bit legacy:
		PCI bus addresses are of the form 0x00000000XXXXXXXX
		host_addr = (0xffffffff << 32) | bus_addr
		bus_addr = (host_addr & 0xffffffff)

	64-bit PAT boxes:
		PCI bus addresses are of the form 0x00000000XXXXXXXX
		lmmio_base is of the form 0xXXXXXXXX00000000
		host_addr = lmmio_base | bus_addr
		bus_addr = (host_addr & 0xffffffff)

My reasoning was that since there's no overlap between bits set in 
lmmio_base and bits set in PCI bus addresses, this should be equivalent to

	host_addr = bus_addr + offset
	bus_addr = host_addr - offset

where

	32-bit boxes:
		offset = 0

	64-bit legacy:
		offset = (0xffffffff << 32)

	64-bit PAT boxes:
		offset = lmmio_base_PA_VIEW - lmmio_base_IO_VIEW
		(for non-Superdome boxes, lmmio_base_IO_VIEW is 0)

> > @@ -1181,7 +1177,7 @@ lba_legacy_resources( struct hp_device *
> >  	r->end = r->start + rsize - 1 ;
> >
> >  	/*
> > -	** XXX FIXME - ignore LBA_ELMMIO_BASE for now
> > +	** XXX FIXME - ignore LBA_LMMIO_BASE for now
>
> I meant ELMMIO_BASE.
>
> But I can't find "ELMMIO" in the Elroy ERS I have (rev 1.4, 08/23/99).
> Offset 0x250 is marked "reserved". I guess one has to have HPUX source
> code access to know about this. *sigh*.

The Mercury ERS describes ELMMIO and says Elroy has it too, but removed it 
from the ERS.  I'll leave the comment as it was except for correcting 
"distrubuted".  I had just thought ELMMIO itself was a typo.

> > @@ -744,6 +744,7 @@ dino_card_init(struct dino_device *dino_
> >
> >  	dino_dev->ioport_addr =  0x00001000;	/* Make believe */
> >  	dino_dev->mmio_addr   =  0xf0800000;	/* FIXME: Make believe */
> > +	dino_dev->hba.mem_space_offset = 0;	/* CPU addrs == bus addrs */
>
> Here, mem_space_offset should be set to -1 since the mmio_addr
> is bogus too. Card-mode dino doesn't route MMIO transaction
> to the PCI bus. At least not until someone writes the code to figure
> out which MMIO ranges are available and programs IOADDR_EN in Dino.
> PDC does this for built-in dino but not for card-mode dino.

Why is -1 preferable to 0?  I'm not interested in making card-mode dino 
MMIO work either; the only reason I set mem_space_offset at all here was 
so that if somebody *did* try to use PCI_{HOST,BUS}_ADDR with such a dino, 
it wouldn't use an uninitialized offset.

> In fact, I'm wondering if pcibios_host_to_bus()even implements
> pcivtobus() correctly.  IIRC, pcivtobus() is to convert *any* Host
> virtual address into and address which can be DMA'd to by the PCI
> device. This mess was replaced with Dynamic DMA mapping in 2.3 and
> it's just taking the drivers a while to catch up.

I think the driver uses pcivtobus() only for CSRs and script memory, not 
or arbitrary host addresses.  Since it doesn't appear in 2.4.4. at all, 
we're in luck, because this was the only use for pcibios_host_to_bus(), so 
we'll be able to just nuke the whole thing.

Bjorn