On Tue, Apr 25, 2017 at 4:19 PM, Christian König <deathsimple@xxxxxxxxxxx> wrote: > From: Christian König <christian.koenig@xxxxxxx> > > Just the defines and helper functions to read the possible sizes of a BAR and > update it's size. > > See https://pcisig.com/sites/default/files/specification_documents/ECN_Resizable-BAR_24Apr2008.pdf > and PCIe r3.1, sec 7.22. > > This is useful for hardware with large local storage (mostly GFX) which only > expose 256MB BARs initially to be compatible with 32bit systems. > +u32 pci_rbar_get_possible_sizes(struct pci_dev *pdev, int bar) > +{ > + unsigned pos, nbars; > + u32 ctrl, cap; > + unsigned i; Are we supposed to use plain 'unsigned' nowadays? I would go with 'unsigned int'. > +} > + * Returns size if found or negativ error code. Typo: negative. > +int pci_rbar_get_current_size(struct pci_dev *pdev, int bar) > +{ > + unsigned pos, nbars; > + u32 ctrl; > + unsigned i; Reversed tree order? > + for (i = 0; i < nbars; ++i, pos += 8) { > + int bar_idx; > + > + pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl); > + bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >> > + PCI_REBAR_CTRL_BAR_IDX_SHIFT; > + if (bar_idx != bar) > + continue; > + > + return (ctrl & PCI_REBAR_CTRL_BAR_SIZE_MASK) >> > + PCI_REBAR_CTRL_BAR_SIZE_SHIFT; > + } This one the same as previous function, the difference only in what is returned. CAre to split static helper function for both? > + return -ENOENT; > +} > +/** > + * pci_rbar_set_size - set a new size for a BAR > + * @dev: PCI device > + * @bar: BAR to set size to > + * @size: new size as defined in the spec (log2(size in bytes) - 20) Not clear is it rounded up / down. I would go with "...in the spec (0=1MB, 19=512GB)". > + * > + * Set the new size of a BAR as defined in the spec (0=1MB, 19=512GB). > + * Returns true if resizing was successful, false otherwise. > + */ > +int pci_rbar_set_size(struct pci_dev *pdev, int bar, int size) > +{ > + unsigned pos, nbars; > + u32 ctrl; > + unsigned i; > + > + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR); > + if (!pos) > + return -ENOTSUPP; > + > + pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl); > + nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >> PCI_REBAR_CTRL_NBAR_SHIFT; > + > + for (i = 0; i < nbars; ++i, pos += 8) { > + int bar_idx; > + > + pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl); > + bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >> > + PCI_REBAR_CTRL_BAR_IDX_SHIFT; > + if (bar_idx != bar) > + continue; Above is duplicating previous. So, static int ..._find_rbar(..., u32 *ctrl) { } Returns: (i.e.) 0 - found, 1 - not found, -ERRNO. ret = _find_rbar(); if (ret < 0) return ret; if (ret) return -ENOENT; ... return 0; So, please refactor. > + > + ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE_MASK; > + ctrl |= size << PCI_REBAR_CTRL_BAR_SIZE_SHIFT; > + pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl); > + return 0; > + } > + > + return -ENOENT; > +} > -#define PCI_REBAR_CTRL_NBAR_MASK (7 << 5) /* mask for # bars */ > -#define PCI_REBAR_CTRL_NBAR_SHIFT 5 /* shift for # bars */ > +#define PCI_REBAR_CTRL_NBAR_MASK (7 << 5) /* mask for # BARs */ > +#define PCI_REBAR_CTRL_NBAR_SHIFT 5 /* shift for # BARs */ I understand why, but I dunno it worth to do. -- With Best Regards, Andy Shevchenko
