Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34081 )
Change subject: device/oprom: Reduce indentation ......................................................................
device/oprom: Reduce indentation
Change-Id: Iadae9221f7ea549e91cdc501155de058c51a982c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/device/oprom/yabel/io.c 1 file changed, 101 insertions(+), 94 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34081/1
diff --git a/src/device/oprom/yabel/io.c b/src/device/oprom/yabel/io.c index 7117a6e..69cbe19 100644 --- a/src/device/oprom/yabel/io.c +++ b/src/device/oprom/yabel/io.c @@ -410,66 +410,69 @@ u32 pci_cfg_read(X86EMU_pioAddr addr, u8 size) { + u32 port_cf8_val = 0; u32 rval = 0xFFFFFFFF; - struct device * dev; - if ((addr >= 0xCFC) && ((addr + size) <= 0xD00)) { - // PCI Configuration Mechanism 1 step 1 - // write to 0xCF8, sets bus, device, function and Config Space offset - // later read from 0xCFC-0xCFF returns the value... - u8 bus, devfn, offs; - u32 port_cf8_val = my_inl(0xCF8); - if ((port_cf8_val & 0x80000000) != 0) { - //highest bit enables config space mapping - bus = (port_cf8_val & 0x00FF0000) >> 16; - devfn = (port_cf8_val & 0x0000FF00) >> 8; - offs = (port_cf8_val & 0x000000FF); - offs += (addr - 0xCFC); // if addr is not 0xcfc, the offset is moved accordingly - DEBUG_PRINTF_INTR("%s(): PCI Config Read from device: bus: %02x, devfn: %02x, offset: %02x\n", - __func__, bus, devfn, offs); + struct device *dev = NULL; + u8 bus, devfn, offs; + + // PCI Configuration Mechanism 1 step 1 + // write to 0xCF8, sets bus, device, function and Config Space offset + // later read from 0xCFC-0xCFF returns the value... + if ((addr >= 0xCFC) && ((addr + size) <= 0xD00)) + port_cf8_val = my_inl(0xCF8); + + if ((port_cf8_val & 0x80000000) == 0) + return rval; + + //highest bit enables config space mapping + bus = (port_cf8_val & 0x00FF0000) >> 16; + devfn = (port_cf8_val & 0x0000FF00) >> 8; + offs = (port_cf8_val & 0x000000FF); + offs += (addr - 0xCFC); // if addr is not 0xcfc, the offset is moved accordingly + DEBUG_PRINTF_INTR("%s(): PCI Config Read from device: bus: %02x, devfn: %02x, offset: %02x\n", + __func__, bus, devfn, offs); #if CONFIG(YABEL_PCI_ACCESS_OTHER_DEVICES) - dev = dev_find_slot(bus, devfn); - DEBUG_PRINTF_INTR("%s(): dev_find_slot() returned: %s\n", - __func__, dev_path(dev)); - if (dev == 0) { - // fail accesses to non-existent devices... + dev = dev_find_slot(bus, devfn); + DEBUG_PRINTF_INTR("%s(): dev_find_slot() returned: %s\n", + __func__, dev_path(dev)); + if (dev == 0) { + // fail accesses to non-existent devices... #else - dev = bios_device.dev; - if ((bus != bios_device.bus) - || (devfn != bios_device.devfn)) { - // fail accesses to any device but ours... + dev = bios_device.dev; + if ((bus != bios_device.bus) + || (devfn != bios_device.devfn)) { + // fail accesses to any device but ours... #endif - printf - ("%s(): Config read access invalid device! bus: %02x (%02x), devfn: %02x (%02x), offs: %02x\n", - __func__, bus, bios_device.bus, devfn, - bios_device.devfn, offs); - SET_FLAG(F_CF); - HALT_SYS(); - return 0; - } else { + printf + ("%s(): Config read access invalid device! bus: %02x (%02x), devfn: %02x (%02x), offs: %02x\n", + __func__, bus, bios_device.bus, devfn, + bios_device.devfn, offs); + SET_FLAG(F_CF); + HALT_SYS(); + return 0; + } else { #if CONFIG(PCI_OPTION_ROM_RUN_YABEL) - switch (size) { - case 1: - rval = pci_read_config8(dev, offs); - break; - case 2: - rval = pci_read_config16(dev, offs); - break; - case 4: - rval = pci_read_config32(dev, offs); - break; - } -#else - rval = - (u32) rtas_pci_config_read(bios_device. - puid, size, - bus, devfn, - offs); -#endif - DEBUG_PRINTF_IO - ("%s(%04x) PCI Config Read @%02x, size: %d --> 0x%08x\n", - __func__, addr, offs, size, rval); - } + switch (size) { + case 1: + rval = pci_read_config8(dev, offs); + break; + case 2: + rval = pci_read_config16(dev, offs); + break; + case 4: + rval = pci_read_config32(dev, offs); + break; } +#else + rval = + (u32) rtas_pci_config_read(bios_device. + puid, size, + bus, devfn, + offs); +#endif + DEBUG_PRINTF_IO + ("%s(%04x) PCI Config Read @%02x, size: %d --> 0x%08x\n", + __func__, addr, offs, size, rval); } return rval; } @@ -477,50 +480,54 @@ void pci_cfg_write(X86EMU_pioAddr addr, u32 val, u8 size) { - if ((addr >= 0xCFC) && ((addr + size) <= 0xD00)) { - // PCI Configuration Mechanism 1 step 1 - // write to 0xCF8, sets bus, device, function and Config Space offset - // later write to 0xCFC-0xCFF sets the value... - u8 bus, devfn, offs; - u32 port_cf8_val = my_inl(0xCF8); - if ((port_cf8_val & 0x80000000) != 0) { - //highest bit enables config space mapping - bus = (port_cf8_val & 0x00FF0000) >> 16; - devfn = (port_cf8_val & 0x0000FF00) >> 8; - offs = (port_cf8_val & 0x000000FF); - offs += (addr - 0xCFC); // if addr is not 0xcfc, the offset is moved accordingly - if ((bus != bios_device.bus) - || (devfn != bios_device.devfn)) { - // fail accesses to any device but ours... - printf - ("Config write access invalid! PCI device %x:%x.%x, offs: %x\n", - bus, devfn >> 3, devfn & 7, offs); + u32 port_cf8_val = 0; + u8 bus, devfn, offs; + + // PCI Configuration Mechanism 1 step 1 + // write to 0xCF8, sets bus, device, function and Config Space offset + // later write to 0xCFC-0xCFF sets the value... + + if ((addr >= 0xCFC) && ((addr + size) <= 0xD00)) + port_cf8_val = my_inl(0xCF8); + + if ((port_cf8_val & 0x80000000) == 0) + return; + + //highest bit enables config space mapping + bus = (port_cf8_val & 0x00FF0000) >> 16; + devfn = (port_cf8_val & 0x0000FF00) >> 8; + offs = (port_cf8_val & 0x000000FF); + offs += (addr - 0xCFC); // if addr is not 0xcfc, the offset is moved accordingly + if ((bus != bios_device.bus) + || (devfn != bios_device.devfn)) { + // fail accesses to any device but ours... + printf + ("Config write access invalid! PCI device %x:%x.%x, offs: %x\n", + bus, devfn >> 3, devfn & 7, offs); #if !CONFIG(YABEL_PCI_FAKE_WRITING_OTHER_DEVICES_CONFIG) - HALT_SYS(); + HALT_SYS(); #endif - } else { + } else { #if CONFIG(PCI_OPTION_ROM_RUN_YABEL) - switch (size) { - case 1: - pci_write_config8(bios_device.dev, offs, val); - break; - case 2: - pci_write_config16(bios_device.dev, offs, val); - break; - case 4: - pci_write_config32(bios_device.dev, offs, val); - break; - } -#else - rtas_pci_config_write(bios_device.puid, - size, bus, devfn, offs, - val); -#endif - DEBUG_PRINTF_IO - ("%s(%04x) PCI Config Write @%02x, size: %d <-- 0x%08x\n", - __func__, addr, offs, size, val); - } + switch (size) { + case 1: + pci_write_config8(bios_device.dev, offs, val); + break; + case 2: + pci_write_config16(bios_device.dev, offs, val); + break; + case 4: + pci_write_config32(bios_device.dev, offs, val); + break; } +#else + rtas_pci_config_write(bios_device.puid, + size, bus, devfn, offs, + val); +#endif + DEBUG_PRINTF_IO + ("%s(%04x) PCI Config Write @%02x, size: %d <-- 0x%08x\n", + __func__, addr, offs, size, val); } }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34081 )
Change subject: device/oprom: Reduce indentation ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/34081/1/src/device/oprom/yabel/io.c File src/device/oprom/yabel/io.c:
https://review.coreboot.org/c/coreboot/+/34081/1/src/device/oprom/yabel/io.c... PS1, Line 432: DEBUG_PRINTF_INTR("%s(): PCI Config Read from device: bus: %02x, devfn: %02x, offset: %02x\n", line over 96 characters
https://review.coreboot.org/c/coreboot/+/34081/1/src/device/oprom/yabel/io.c... PS1, Line 447: ("%s(): Config read access invalid device! bus: %02x (%02x), devfn: %02x (%02x), offs: %02x\n", line over 96 characters
https://review.coreboot.org/c/coreboot/+/34081/1/src/device/oprom/yabel/io.c... PS1, Line 453: } else { else is not generally useful after a break or return
https://review.coreboot.org/c/coreboot/+/34081/1/src/device/oprom/yabel/io.c... PS1, Line 455: switch (size) { switch and case should be at the same indent
https://review.coreboot.org/c/coreboot/+/34081/1/src/device/oprom/yabel/io.c... PS1, Line 469: puid, size, Avoid multiple line dereference - prefer 'bios_device.puid'
https://review.coreboot.org/c/coreboot/+/34081/1/src/device/oprom/yabel/io.c... PS1, Line 512: switch (size) { switch and case should be at the same indent
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34081 )
Change subject: device/oprom: Reduce indentation ......................................................................
Patch Set 1: Code-Review+1
Hello Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34081
to look at the new patch set (#2).
Change subject: device/oprom: Reduce indentation ......................................................................
device/oprom: Reduce indentation
Change-Id: Iadae9221f7ea549e91cdc501155de058c51a982c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/device/oprom/yabel/io.c 1 file changed, 101 insertions(+), 94 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34081/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34081 )
Change subject: device/oprom: Reduce indentation ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34081/2/src/device/oprom/yabel/io.c File src/device/oprom/yabel/io.c:
https://review.coreboot.org/c/coreboot/+/34081/2/src/device/oprom/yabel/io.c... PS2, Line 432: DEBUG_PRINTF_INTR("%s(): PCI Config Read from device: bus: %02x, devfn: %02x, offset: %02x\n", line over 96 characters
https://review.coreboot.org/c/coreboot/+/34081/2/src/device/oprom/yabel/io.c... PS2, Line 447: ("%s(): Config read access invalid device! bus: %02x (%02x), devfn: %02x (%02x), offs: %02x\n", line over 96 characters
https://review.coreboot.org/c/coreboot/+/34081/2/src/device/oprom/yabel/io.c... PS2, Line 453: } else { else is not generally useful after a break or return
https://review.coreboot.org/c/coreboot/+/34081/2/src/device/oprom/yabel/io.c... PS2, Line 469: puid, size, Avoid multiple line dereference - prefer 'bios_device.puid'
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34081 )
Change subject: device/oprom: Reduce indentation ......................................................................
Patch Set 2:
Testing on CB:34083 validates this.
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34081 )
Change subject: device/oprom: Reduce indentation ......................................................................
Patch Set 2:
Patch Set 2:
Testing on CB:34083 validates this.
How I could test CB:34081 and CB:34083 at the same time? I tried doing
git fetch "https://mikebdp@review.coreboot.org/a/coreboot" refs/changes/83/34083/2 && git checkout FETCH_HEAD # ^^^ apply CB:34083 wget https://review.coreboot.org/changes/34081/revisions/2/patch?zip unzip d98eb02.diff.zip patch -p1 < ./d98eb02.diff # <--- apply this CB:34081 patch - revision 2
but got
patching file src/device/oprom/yabel/io.c Hunk #1 FAILED at 410. Hunk #2 FAILED at 477. 2 out of 2 hunks FAILED -- saving rejects to file src/device/oprom/yabel/io.c.rej
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34081 )
Change subject: device/oprom: Reduce indentation ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Testing on CB:34083 validates this.
How I could test CB:34081 and CB:34083 at the same time? I tried doing
When you did a checkout on CB:34083 you already picked up the parent commits. In contrast, a cherry-pick will only fetch the single commit.
Run 'git log --oneline' to see what you are actually building.
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34081 )
Change subject: device/oprom: Reduce indentation ......................................................................
Patch Set 2: Code-Review+2
Patch Set 2:
Patch Set 2:
Patch Set 2:
Testing on CB:34083 validates this.
How I could test CB:34081 and CB:34083 at the same time? I tried doing
When you did a checkout on CB:34083 you already picked up the parent commits. In contrast, a cherry-pick will only fetch the single commit.
Run 'git log --oneline' to see what you are actually building.
Thank you very much for clarifying.
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34081 )
Change subject: device/oprom: Reduce indentation ......................................................................
device/oprom: Reduce indentation
Change-Id: Iadae9221f7ea549e91cdc501155de058c51a982c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34081 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Mike Banon mikebdp2@gmail.com --- M src/device/oprom/yabel/io.c 1 file changed, 101 insertions(+), 94 deletions(-)
Approvals: build bot (Jenkins): Verified Mike Banon: Looks good to me, approved
diff --git a/src/device/oprom/yabel/io.c b/src/device/oprom/yabel/io.c index 7117a6e..2b384ea 100644 --- a/src/device/oprom/yabel/io.c +++ b/src/device/oprom/yabel/io.c @@ -410,66 +410,69 @@ u32 pci_cfg_read(X86EMU_pioAddr addr, u8 size) { + u32 port_cf8_val = 0; u32 rval = 0xFFFFFFFF; - struct device * dev; - if ((addr >= 0xCFC) && ((addr + size) <= 0xD00)) { - // PCI Configuration Mechanism 1 step 1 - // write to 0xCF8, sets bus, device, function and Config Space offset - // later read from 0xCFC-0xCFF returns the value... - u8 bus, devfn, offs; - u32 port_cf8_val = my_inl(0xCF8); - if ((port_cf8_val & 0x80000000) != 0) { - //highest bit enables config space mapping - bus = (port_cf8_val & 0x00FF0000) >> 16; - devfn = (port_cf8_val & 0x0000FF00) >> 8; - offs = (port_cf8_val & 0x000000FF); - offs += (addr - 0xCFC); // if addr is not 0xcfc, the offset is moved accordingly - DEBUG_PRINTF_INTR("%s(): PCI Config Read from device: bus: %02x, devfn: %02x, offset: %02x\n", - __func__, bus, devfn, offs); + struct device *dev = NULL; + u8 bus, devfn, offs; + + // PCI Configuration Mechanism 1 step 1 + // write to 0xCF8, sets bus, device, function and Config Space offset + // later read from 0xCFC-0xCFF returns the value... + if ((addr >= 0xCFC) && ((addr + size) <= 0xD00)) + port_cf8_val = my_inl(0xCF8); + + if ((port_cf8_val & 0x80000000) == 0) + return rval; + + //highest bit enables config space mapping + bus = (port_cf8_val & 0x00FF0000) >> 16; + devfn = (port_cf8_val & 0x0000FF00) >> 8; + offs = (port_cf8_val & 0x000000FF); + offs += (addr - 0xCFC); // if addr is not 0xcfc, the offset is moved accordingly + DEBUG_PRINTF_INTR("%s(): PCI Config Read from device: bus: %02x, devfn: %02x, offset: %02x\n", + __func__, bus, devfn, offs); #if CONFIG(YABEL_PCI_ACCESS_OTHER_DEVICES) - dev = dev_find_slot(bus, devfn); - DEBUG_PRINTF_INTR("%s(): dev_find_slot() returned: %s\n", - __func__, dev_path(dev)); - if (dev == 0) { - // fail accesses to non-existent devices... + dev = dev_find_slot(bus, devfn); + DEBUG_PRINTF_INTR("%s(): dev_find_slot() returned: %s\n", + __func__, dev_path(dev)); + if (dev == 0) { + // fail accesses to non-existent devices... #else - dev = bios_device.dev; - if ((bus != bios_device.bus) - || (devfn != bios_device.devfn)) { - // fail accesses to any device but ours... + dev = bios_device.dev; + if ((bus != bios_device.bus) + || (devfn != bios_device.devfn)) { + // fail accesses to any device but ours... #endif - printf - ("%s(): Config read access invalid device! bus: %02x (%02x), devfn: %02x (%02x), offs: %02x\n", - __func__, bus, bios_device.bus, devfn, - bios_device.devfn, offs); - SET_FLAG(F_CF); - HALT_SYS(); - return 0; - } else { + printf + ("%s(): Config read access invalid device! bus: %02x (%02x), devfn: %02x (%02x), offs: %02x\n", + __func__, bus, bios_device.bus, devfn, + bios_device.devfn, offs); + SET_FLAG(F_CF); + HALT_SYS(); + return 0; + } else { #if CONFIG(PCI_OPTION_ROM_RUN_YABEL) - switch (size) { - case 1: - rval = pci_read_config8(dev, offs); - break; - case 2: - rval = pci_read_config16(dev, offs); - break; - case 4: - rval = pci_read_config32(dev, offs); - break; - } -#else - rval = - (u32) rtas_pci_config_read(bios_device. - puid, size, - bus, devfn, - offs); -#endif - DEBUG_PRINTF_IO - ("%s(%04x) PCI Config Read @%02x, size: %d --> 0x%08x\n", - __func__, addr, offs, size, rval); - } + switch (size) { + case 1: + rval = pci_read_config8(dev, offs); + break; + case 2: + rval = pci_read_config16(dev, offs); + break; + case 4: + rval = pci_read_config32(dev, offs); + break; } +#else + rval = + (u32) rtas_pci_config_read(bios_device. + puid, size, + bus, devfn, + offs); +#endif + DEBUG_PRINTF_IO + ("%s(%04x) PCI Config Read @%02x, size: %d --> 0x%08x\n", + __func__, addr, offs, size, rval); } return rval; } @@ -477,50 +480,54 @@ void pci_cfg_write(X86EMU_pioAddr addr, u32 val, u8 size) { - if ((addr >= 0xCFC) && ((addr + size) <= 0xD00)) { - // PCI Configuration Mechanism 1 step 1 - // write to 0xCF8, sets bus, device, function and Config Space offset - // later write to 0xCFC-0xCFF sets the value... - u8 bus, devfn, offs; - u32 port_cf8_val = my_inl(0xCF8); - if ((port_cf8_val & 0x80000000) != 0) { - //highest bit enables config space mapping - bus = (port_cf8_val & 0x00FF0000) >> 16; - devfn = (port_cf8_val & 0x0000FF00) >> 8; - offs = (port_cf8_val & 0x000000FF); - offs += (addr - 0xCFC); // if addr is not 0xcfc, the offset is moved accordingly - if ((bus != bios_device.bus) - || (devfn != bios_device.devfn)) { - // fail accesses to any device but ours... - printf - ("Config write access invalid! PCI device %x:%x.%x, offs: %x\n", - bus, devfn >> 3, devfn & 7, offs); + u32 port_cf8_val = 0; + u8 bus, devfn, offs; + + // PCI Configuration Mechanism 1 step 1 + // write to 0xCF8, sets bus, device, function and Config Space offset + // later write to 0xCFC-0xCFF sets the value... + + if ((addr >= 0xCFC) && ((addr + size) <= 0xD00)) + port_cf8_val = my_inl(0xCF8); + + if ((port_cf8_val & 0x80000000) == 0) + return; + + //highest bit enables config space mapping + bus = (port_cf8_val & 0x00FF0000) >> 16; + devfn = (port_cf8_val & 0x0000FF00) >> 8; + offs = (port_cf8_val & 0x000000FF); + offs += (addr - 0xCFC); // if addr is not 0xcfc, the offset is moved accordingly + if ((bus != bios_device.bus) + || (devfn != bios_device.devfn)) { + // fail accesses to any device but ours... + printf + ("Config write access invalid! PCI device %x:%x.%x, offs: %x\n", + bus, devfn >> 3, devfn & 7, offs); #if !CONFIG(YABEL_PCI_FAKE_WRITING_OTHER_DEVICES_CONFIG) - HALT_SYS(); + HALT_SYS(); #endif - } else { + } else { #if CONFIG(PCI_OPTION_ROM_RUN_YABEL) - switch (size) { - case 1: - pci_write_config8(bios_device.dev, offs, val); - break; - case 2: - pci_write_config16(bios_device.dev, offs, val); - break; - case 4: - pci_write_config32(bios_device.dev, offs, val); - break; - } -#else - rtas_pci_config_write(bios_device.puid, - size, bus, devfn, offs, - val); -#endif - DEBUG_PRINTF_IO - ("%s(%04x) PCI Config Write @%02x, size: %d <-- 0x%08x\n", - __func__, addr, offs, size, val); - } + switch (size) { + case 1: + pci_write_config8(bios_device.dev, offs, val); + break; + case 2: + pci_write_config16(bios_device.dev, offs, val); + break; + case 4: + pci_write_config32(bios_device.dev, offs, val); + break; } +#else + rtas_pci_config_write(bios_device.puid, + size, bus, devfn, offs, + val); +#endif + DEBUG_PRINTF_IO + ("%s(%04x) PCI Config Write @%02x, size: %d <-- 0x%08x\n", + __func__, addr, offs, size, val); } }
-
build bot (Jenkins) (Code Review) -
Kyösti Mälkki (Code Review)
-
Mike Banon (Code Review)
-
Paul Menzel (Code Review)