On 2022-02-14 12:50 a.m., Jonathan Gray wrote:
On Mon, Feb 14, 2022 at 12:05:44AM -0700, Ted Bullock wrote:
On 2022-02-13 11:02 p.m., Jonathan Gray wrote:
On Sun, Feb 13, 2022 at 12:22:38PM -0700, Ted Bullock wrote:
On 2022-02-12 6:46 p.m., Jonathan Gray wrote:
I will review further when you drop the function.
Alright try this again,
I have committed some parts of this, with one commit per specific issue.
pa_memex NULL test
sparc64 ifndef
drm_attach_pci return test
the result of pci_mapreg_type() is already fine as it does
_PCI_MAPREG_TYPEBITS() which which masks the bits
I still find this diff hard to follow as you are moving code around.
The arrays of bar information can be dropped.
When you cherrypicked fixes, you missed the for loop as per the initial
mail, the type checks are incorrect and won't match. You need the helper
macros since those types are bitmaps not types. This has been like this
since 1.71 (Oct 2020)
for (i = PCI_MAPREG_START; i < PCI_MAPREG_END; i += 4) {
type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, i);
if (type == PCI_MAPREG_TYPE_IO) {
^^^^
pci_mapreg_map(pa, i, type, 0, NULL,
&rdev->rio_mem, NULL, &rdev->rio_mem_size, 0);
break;
}
if (type == PCI_MAPREG_MEM_TYPE_64BIT)
^^^^
i += 4;
}
type = _PCI_MAPREG_TYPEBITS(pci_conf_read(pc, tag, reg));
if (type == 1) {
pci_mapreg_map();
break;
}
if (type == 4)
i += 4;
---
x = pci_conf_read(pc, tag, reg);
if ((x & 1) == 1)
type = x & 1;
else
type = x & 7;
if (type == 1) {
pci_mapreg_map();
break;
}
if (type == 4)
i += 4;
which other bits do you expect?
001 io space
000 32-bit mem space
100 64-bit mem space
See below quote
if (type == PCI_MAPREG_TYPE_IO) {
^^^^
This is a bitmap, you cannot use it like this, the correct usage
would be PCI_MAPREG_TYPE(type) == PCI_MAPREG_TYPE_IO
pci_mapreg_map(pa, i, type, 0, NULL,
&rdev->rio_mem, NULL, &rdev->rio_mem_size, 0);
break;
}
if (type == PCI_MAPREG_MEM_TYPE_64BIT)
^^^^
Same as above, this is incorrect usage
i += 4;
A note about this for loop. This loop was broken in revision 1.72 to
support radeon devices on POWER9 systems. These registers are available
(I believe) through the MMIO BAR so people haven't noticed that it's
broken. I don't know if ALL devices work like that, though certainly
most do, if so this entire loop could possibly be removed and just rely
on MMIO access to these registers.
^^^^
From my initial mail on the issues I saw. Is this incorrect and isn't
PCI_MAPREG_TYPE needed for checking type? I think so, but maybe I'm wrong.
--
Ted Bullock <[email protected]>
