[parisc-linux] Using PAT_IO calls for PCI config space reads and writes.

[parisc-linux] Using PAT_IO calls for PCI config space reads and writes.

[Naresh Kumar]

Hi all,

When I was trying to bring up PA-Linux-2.4 on some of the newer boxes, I
discovered that reads/writes from the PCI config space were failing (
system crashes and hangs during the boot). Currently, reads and writes
to the config space happen through memory loads and stores to PCI config
addresses directly  (lba_cfg_[read|write]##size in lba_pci.c ). Grant
Grundler advised me to use PDC_PAT_IO calls instead, for PAT based
systems, since they are more reliable and take care of border cases on
newer systems. I have made the changes and tested them on an L-Class
system. I am posting the diff of the files I have changed. The changes
have been made to three files:

1. arch/parisc/kernel/firmware.c - Rev 1.47
2. arch/parisc/kernel/lba_pci.c - Rev 1.54
3. include/asm-parisc/pdc.h - Rev 1.48

Kindly let me know your comments:

--------------------START------------------------------------------------------------------------------

--- lba_pci.c.1.54       Fri Jan 23 15:47:41 2004
+++ lba_pci.c.modified   Fri Jan 23 15:53:15 2004
@@ -504,6 +504,13 @@ lba_rd_cfg(struct lba_device *d, u32 tok
        return(data);
 }

+#ifdef __LP64__
+#define PAT_CFG_READ(a,b,c)    pdc_pat_io_pci_cfg_read(a,b,c)
+#define PAT_CFG_WRITE(a,b,c)   pdc_pat_io_pci_cfg_write(a,b,c)
+#else
+#define PAT_CFG_READ(a,b,c)
+#define PAT_CFG_WRITE(a,b,c)
+#endif

 #define LBA_CFG_RD(size, mask) \
 static int lba_cfg_read##size (struct pci_dev *dev, int pos, u##size
*data) \
@@ -512,6 +519,21 @@ static int lba_cfg_read##size (struct pc
        u32 local_bus = (dev->bus->parent == NULL) ? 0 :
dev->bus->secondary; \
        u32 tok = LBA_CFG_TOK(local_bus,dev->devfn); \
  \
+       if (is_pdc_pat()) { \
+               int ret; \
+               tok = LBA_CFG_TOK(dev->bus->number,dev->devfn); \
+               ret = PAT_CFG_READ((tok | pos ), \
+                                        sizeof(u##size), (u##size *)
data); \
+               if ( ret == 0 ) { \
+                       DBG_CFG("%s(%s+%2x) -> 0x%x (c)\n",
__FUNCTION__, dev->slot_name, pos, *data
); \
+                       return(*data == (u##size) -1); \
+               } else { \
+                       DBG_CFG("LBA: CFG read failed: ret = %d,
d->hba.base_addr = 0x%lx\n", \
+                                               ret, d->hba.base_addr );
\
+                       return (1); \
+               } \
+       } \
+ \
 /* FIXME: B2K/C3600 workaround is always use old method... */ \
        /* if (!LBA_TR4PLUS(d) && !LBA_SKIP_PROBE(d)) */ { \
                /* original - Generate config cycle on broken elroy \
@@ -611,6 +633,11 @@ static int lba_cfg_write##size (struct p
        } \
  \
        DBG_CFG("%s(%s+%2x) = 0x%x (c)\n", __FUNCTION__, dev->slot_name,
pos, data); \
+       if (is_pdc_pat()) { \
+               tok = LBA_CFG_TOK(dev->bus->number,dev->devfn); \
+               PAT_CFG_WRITE((tok | pos ), sizeof(u##size), (u##size
*)&data); \
+               return 0; \
+       } \
        /* Basic Algorithm */ \
        LBA_CFG_TR4_ADDR_SETUP(d, tok | pos); \
        WRITE_REG##size(data, d->hba.base_addr + LBA_PCI_CFG_DATA + (pos
& mask)); \
-------------------------------------END---------------------------------------------------------------

------------------------------------START-------------------------------------------------------------

--- firmware.c.1.47      Fri Jan 23 16:57:51 2004
+++ firmware.c.modified   Tue Jan 27 12:32:43 2004
@@ -1035,6 +1035,46 @@ int pdc_pat_pd_get_addr_map(unsigned lon

        return retval;
 }
+
+/**
+ * pdc_pat_io_pci_cfg_read - Read PCI configuration space.
+ * @pci_addr: PCI configuration space address for which the read
request is being made.
+ * @pci_size: Size of read in bytes. Valid values are 1, 2, and 4.
+ * @mem_addr: Pointer to return memory buffer.
+ *
+ */
+int pdc_pat_io_pci_cfg_read(unsigned long pci_addr, int pci_size, void
*mem_addr)
+{
+       int retval;
+       spin_lock_irq(&pdc_lock);
+       retval = mem_pdc_call(PDC_PAT_IO, PDC_PAT_IO_PCI_CONFIG_READ,
__pa(pdc_result),
+                             pci_addr, pci_size);
+       memcpy((char *)mem_addr, (char *) ((char *)pdc_result +
(sizeof(unsigned long) - pci_size))
, pci_size);
+       spin_unlock_irq(&pdc_lock);
+
+       return retval;
+}
+
+/**
+ * pdc_pat_io_pci_cfg_write - Retrieve information about memory address
ranges.
+ * @pci_addr: PCI configuration space address for which the write
request is being made.
+ * @pci_size: Size of write in bytes. Valid values are 1, 2, and 4.
+ * @value: Pointer to 1, 2, or 4 byte value in low order end of
argument to be
+ *         written to PCI Config space.
+ *
+ */
+int pdc_pat_io_pci_cfg_write(unsigned long pci_addr, int pci_size, void
*value)
+{
+       int retval;
+       unsigned long *val_ptr;
+       spin_lock_irq(&pdc_lock);
+       memcpy((char *)((char *)val_ptr + (sizeof(unsigned long) -
pci_size)), (char *)value, pci_si
ze);
+       retval = mem_pdc_call(PDC_PAT_IO, PDC_PAT_IO_PCI_CONFIG_WRITE,
pci_addr,
+                             pci_size, *val_ptr);
+       spin_unlock_irq(&pdc_lock);
+
+       return retval;
+}
 #endif /* __LP64__ */
-------------------------------------END---------------------------------------------------------------

------------------------------------START-------------------------------------------------------------

--- pdc.h.1.48   Fri Jan 23 16:57:52 2004
+++ pdc.h.modified        Fri Jan 23 12:21:46 2004
@@ -972,6 +972,8 @@ int pdc_pat_get_irt_size(unsigned long *
 int pdc_pat_get_irt(void *r_addr, unsigned long cell_num);
 int pdc_pat_pd_get_addr_map(unsigned long *actual_len, void *mem_addr,
                            unsigned long count, unsigned long offset);
+int pdc_pat_io_pci_cfg_read(unsigned long pci_addr, int pci_size, void
*mem_addr);
+int pdc_pat_io_pci_cfg_write(unsigned long pci_addr, int pci_size, void
*value);

 /******************************************************************** *

 *PDC_PAT_CELL[Return Cell Module] memaddr[0] conf_base_addr
-------------------------------------END---------------------------------------------------------------

A couple of questions:
1. In the definition of 'pdc_pat_io_pci_cfg_read( )' and
'pdc_pat_io_pci_cfg_write( )' above, can I  use 'cpu_to_le64( )' kind of
function instead of ordering the bytes manually in the 'memcpy( )'?
2. Can these changes be propagated to 2.6 also?

Thanks,
Naresh.

Re: [parisc-linux] Using PAT_IO calls for PCI config space reads and writes.

[Grant Grundler]

On Tue, Jan 27, 2004 at 02:09:38PM +0530, Naresh Kumar wrote:
> When I was trying to bring up PA-Linux-2.4 on some of the newer boxes, I
> discovered that reads/writes from the PCI config space were failing (

Naresh,
Awesome!
Thanks for pulling this together and sending it out.

...
> have been made to three files:
> 
> 1. arch/parisc/kernel/firmware.c - Rev 1.47
> 2. arch/parisc/kernel/lba_pci.c - Rev 1.54
> 3. include/asm-parisc/pdc.h - Rev 1.48
> 
> Kindly let me know your comments:
> 
> --------------------START------------------------------------------------------------------------------
> 
> --- lba_pci.c.1.54       Fri Jan 23 15:47:41 2004
> +++ lba_pci.c.modified   Fri Jan 23 15:53:15 2004

The diff should apply with:
	cd /usr/src/linux-2.4; patch -p1 < ~/diff-2.4-PAT_CFG

This patch doesn't do that or even come close.
Can you send a new patch generated with:
	cd /usr/src/linux-2.4 ; cvs diff -uNp  > ~/diff-2.4-PAT_CFG

thanks,
grant

Re: [parisc-linux] Using PAT_IO calls for PCI config space reads and writes.

[Matthew Wilcox]

On Tue, Jan 27, 2004 at 02:09:38PM +0530, Naresh Kumar wrote:
> --------------------START------------------------------------------------------------------------------
> 
> --- lba_pci.c.1.54       Fri Jan 23 15:47:41 2004
> +++ lba_pci.c.modified   Fri Jan 23 15:53:15 2004
> @@ -504,6 +504,13 @@ lba_rd_cfg(struct lba_device *d, u32 tok
>         return(data);
>  }
> 
> +#ifdef __LP64__
> +#define PAT_CFG_READ(a,b,c)    pdc_pat_io_pci_cfg_read(a,b,c)
> +#define PAT_CFG_WRITE(a,b,c)   pdc_pat_io_pci_cfg_write(a,b,c)
> +#else
> +#define PAT_CFG_READ(a,b,c)
> +#define PAT_CFG_WRITE(a,b,c)
> +#endif

Hmm.  Why the abstraction?  The normal way to do this is to define dummy
functions when they can't be called.  See include/asm-parisc/pdc.h:

#else /* !__LP64__ */
/* No PAT support for 32-bit kernels...sorry */
#define pdc_pat_get_irt_size(num_entries, cell_numn)    PDC_BAD_PROC
#define pdc_pat_get_irt(r_addr, cell_num)       PDC_BAD_PROC

I actually think the right way to handle this is probably like this:

#define pat_cfg_addr(bus, devfn, addr) ((bus << 16) | (devfn << 8) | addr)

static int pat_cfg_read(struct pci_bus *bus, unsigned int devfn, int pos, int size, u32 *data)
{
	int tok = pat_cfg_addr(bus->number, devfn, pos);
	int ret = pdc_pat_io_pci_cfg_read(tok, size, &data);
	return (ret == SUCCESS);
}

... same for write here ...

static struct pci_ops pat_cfg_ops = {
	.read = pat_cfg_read,
	.write = pat_cfg_write,
};

And then choose whether to use pat_cfg_ops or lba_cfg_ops at probe time.

> +/**
> + * pdc_pat_io_pci_cfg_read - Read PCI configuration space.
> + * @pci_addr: PCI configuration space address for which the read
> request is being made.
> + * @pci_size: Size of read in bytes. Valid values are 1, 2, and 4.
> + * @mem_addr: Pointer to return memory buffer.
> + *
> + */
> +int pdc_pat_io_pci_cfg_read(unsigned long pci_addr, int pci_size, void
> *mem_addr)
> +{
> +       int retval;
> +       spin_lock_irq(&pdc_lock);
> +       retval = mem_pdc_call(PDC_PAT_IO, PDC_PAT_IO_PCI_CONFIG_READ,
> __pa(pdc_result),
> +                             pci_addr, pci_size);
> +       memcpy((char *)mem_addr, (char *) ((char *)pdc_result +
> (sizeof(unsigned long) - pci_size))
> , pci_size);
> +       spin_unlock_irq(&pdc_lock);
> +
> +       return retval;
> +}

I don't see why you need a memcpy, the data should be sufficiently aligned.
Casting to the right data type should be enough.

> A couple of questions:
> 1. In the definition of 'pdc_pat_io_pci_cfg_read( )' and
> 'pdc_pat_io_pci_cfg_write( )' above, can I  use 'cpu_to_le64( )' kind of
> function instead of ordering the bytes manually in the 'memcpy( )'?
> 2. Can these changes be propagated to 2.6 also?

You should be developing against 2.6 in the first place.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [parisc-linux] Using PAT_IO calls for PCI config space reads and writes.

[Grant Grundler]

On Tue, Jan 27, 2004 at 06:43:45PM +0000, Matthew Wilcox wrote:
> > 2. Can these changes be propagated to 2.6 also?
> 
> You should be developing against 2.6 in the first place.

Should - but they don't about 2.6.  At least not yet.
I'll deal with forward porting the changes once your comments are integrated.
Naresh, can you take care of that and resubmit?

thanks,
grant

Re: [parisc-linux] Using PAT_IO calls for PCI config space reads and writes.

[Matthew Wilcox]

On Tue, Jan 27, 2004 at 12:22:12PM -0700, Grant Grundler wrote:
> On Tue, Jan 27, 2004 at 06:43:45PM +0000, Matthew Wilcox wrote:
> > > 2. Can these changes be propagated to 2.6 also?
> > 
> > You should be developing against 2.6 in the first place.
> 
> Should - but they don't about 2.6.  At least not yet.
> I'll deal with forward porting the changes once your comments are integrated.
> Naresh, can you take care of that and resubmit?

Their development would actually go a lot easier if they worked on 2.6.
Compare lba_pci.c between 2.4 and 2.6 -- the new one is a *lot* easier
to work on.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain