Re: PCI extended configuration support

[Christos Zoulas]

On Sep 28,  7:51pm, msai...@execsw.org (Masanobu SAITOH) wrote:
-- Subject: Re: PCI extended configuration support

| On 2015/09/27 11:03, Christos Zoulas wrote:
| > In article <5604d1be.3080...@execsw.org>,
| > Masanobu SAITOH  <msai...@execsw.org> wrote:
| >> On 2015/09/11 19:44, Masanobu SAITOH wrote:
| >>
| >> More machines support the extended configuration area than before.
| >>
| >>   Is it OK to commit?
| > 
| > LGTM, but since this is not performance critical can't we centralize the
| > range test and do it in a wrapper?
| 
|  We have no good idea... :-(

I meant instead of editing each of the drivers edit the pci_machdep.h file
of each arch from:

#define pci_conf_read(c, t, r)                                          \
    (*(c)->pc_conf_read)((c)->pc_conf_v, (t), (r))
#define pci_conf_write(c, t, r, v)                                      \
    (*(c)->pc_conf_write)((c)->pc_conf_v, (t), (r), (v))

to:

static __inline pcireg_t pci_conf_read(void *_cpv, pcitag_t _tag, int _offset) {
        if ((unsigned int)_offset >= PCI_CONF_SIZE)
                return (pcireg_t)-1;
        return *((c)->pc_conf_read)(
            ((struct <arch>_pci_chipset *)_cpv)->pc_conf_v, _tag, _offset);
}

static __inline void pci_conf_write(void *_cpv, pcitag_t _tag, int _offset) {
        if ((unsigned int)_offset >= PCI_CONF_SIZE)
                return;
        return *((c)->pc_conf_write)(
            ((struct <arch>_pci_chipset *)_cpv)->pc_conf_v, _tag, _offset);
}

Or you could even make a macro of it in pci_var.h like:

#define PCI_ARCH_CONF_READ(c, t, r) \
        do { \
                if ((unsigned int)(r) >= PCI_CONF_SIZE) \
                        return (pcireg_t) -1; \
                return *((c)->pc_conf_read)((c), (t), (r)); \
        while (/*CONSTCOND*/0)

And then change the macro in arch/include/pci_machdep.h to:

#define pci_conf_read(c, t, r) PCI_ARCH_CONF_READ(c, t, r)

Or you could just centralize the machdep macro to pci_var.h, and put it
around ifdefs so that the machdep code can override it:

#ifndef pci_conf_read
#define pci_conf_read(c, t, r) \
        do { \
                if ((unsigned int)(r) >= PCI_CONF_SIZE) \
                        return (pcireg_t) -1; \
                return *((c)->pc_conf_read)((c), (t), (r)); \
        while (/*CONSTCOND*/0)
#endif

| > Plus this can't be negative so perhaps
| > it should be unsigned or checked for < 0 too??
| > 
| > +   if (reg >= PCI_CONF_SIZE)
| > +           return (pcireg_t) -1;
| > +
| 
|  Done.

I think you missed some...

| > Finally a few more constants could be defined instead
| > of hard-coded values....
| > 
| > +   switch (pcie_devtype) {
| > +   case 0x4:       /* Root Port of PCI Express Root Complex */
| > +   case 0xa:       /* Root Complex Event Collector */
| 
|  Done.

Thanks!

christos