On 2015/09/28 22:41, Christos Zoulas wrote:
> 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
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 (msai...@execsw.org
msai...@netbsd.org)
