Re: PCI extended configuration support

[Masanobu SAITOH <msaitoh%execsw.org@localhost>]

On 2015/09/28 22:41, Christos Zoulas wrote:
> On Sep 28,  7:51pm, msaitoh%execsw.org@localhost (Masanobu SAITOH) wrote:
> -- Subject: Re: PCI extended configuration support
> 
> | On 2015/09/27 11:03, Christos Zoulas wrote:
> | > In article <5604D1BE.3080502%execsw.org@localhost>,
> | > Masanobu SAITOH  <msaitoh%execsw.org@localhost> 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

 The size of accessible area is not fixed and it's machine
(SoC or implement of PCI bus) dependent, so it should be done
in ((c)->pc_conf_read)(). Or use function like:

 		if ((unsigned int)(r) >= pci_conf_size()) \
 			return (pcireg_t) -1; \

 pci_conf_read() have to call chip dependent function. I think
it's not good idea to call chip dependent function in pci_conf_size()
and ((c)->pci_conf_read)() twice. And, NOT all archs have pc_conf_read().

 I think writing pci's commom API function with a macro is not good for
kernel module.

> | > 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...

 Fixed. Thanks.

> | > 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

 Final diff:

 http://ftp.netbsd.org/pub/NetBSD/misc/nonaka/tmp/20150929-nbsd-pci-extconf-support.diff


-- 
-----------------------------------------------
                SAITOH Masanobu (msaitoh%execsw.org@localhost
                                 msaitoh%netbsd.org@localhost)