[coreboot] Re: [RFC] i945 problematic code for IGD device enable, BUG?

[Angel Pons]

Hi Petr,

On Sun, Apr 24, 2022 at 7:33 AM Petr Cvek  wrote:
> Hello again :-D,
>
> I'm working on a code for a simultaneous use of IGD (GMA950) and x16 PCIe slot GPU. I've made some success, but the code which handles the IGD initialization is really weird.

If your PCIe device is a graphics card, you could try removing this
condition in early_init.c:

if (reg32 == 0x030000) {
        printk(BIOS_DEBUG, "PCIe device is VGA. Disabling IGD.\n");
        reg16 = (1 << 1);
        pci_write_config16(HOST_BRIDGE, GGC, reg16);

pci_and_config32(HOST_BRIDGE, DEVEN, ~(DEVEN_D2F0 | DEVEN_D2F1));
    }
> The IGD is initialized by DEVEN register (DEVEN_D2F0 and DEVEN_D2F1 bits), which is first written in i945_setup_pci_express_x16(). Few times before that the DEVEN_D2F0 and DEVEN_D2F1 bits are already tested during sdram_initialize() code. Also after the reset these bits are initialized to 1. This will cause the test from:
>
> https://elixir.bootlin.com/coreboot/4.16/source/src/northbridge/intel/i945/r...
>     if (!(pci_read_config8(HOST_BRIDGE, DEVEN) & (DEVEN_D2F0 | DEVEN_D2F1)))
>             integrated_graphics = false;
>
> to be effectively always -> if (0)

Well, there are i945 versions without integrated graphics (82945P,
82945PL, 82945PM). I haven't tested it, but I'm pretty sure the
DEVEN_D2F0 and DEVEN_D2F1 bits are always zero on these northbridges.
> also even when DEVEN_D2F0 is zeroed after chipset reset, the code located after a few lines:
>
> https://elixir.bootlin.com/coreboot/4.16/source/src/northbridge/intel/i945/r...
>     pci_or_config8(IGD_DEV, 0xc1, 1 << 2);
>
> will have a problem to work correctly. IGD_DEV is PCIe device enabled by DEVEN_D2F0 (setting DEVEN_D2F0 to 0 will remove IGD_DEV from the PCIe bus). It seems to not cause problems, but I didn't test it extensively.

This write has no effect if IGD_DEV is disabled by DEVEN. Looks like
this magic 0xc1 register is called UPMC4, but it's undocumented.
> Also the test in northbridge_get_tseg_base() (used everytime top of the memory is needed):
>
> https://elixir.bootlin.com/coreboot/4.16/source/src/northbridge/intel/i945/m...
>     if (pci_read_config8(HOST_BRIDGE, DEVEN) & (DEVEN_D2F0 | DEVEN_D2F1))
>             /* IGD enabled, get top of Memory from BSM register */
>             tom = pci_read_config32(IGD_DEV, BSM);
>
> If only DEVEN_D2F1 is enabled, the test will succeed, but as the IGD_DEV is now disabled by DEVEN_D2F0 it will read garbage data for the global "top of the memory" value.

Having D2F1 enabled without D2F0 being enabled is not allowed by the
PCI specification. Function 0 of multi-function devices must always be
implemented.
> I've sort of solved the missing initialization problem with CMOS config entry in mainboard_romstage_entry():
>     switch (get_uint_option("igd_en", 1)) {
>             case 0:
>                     //disable
>                     pci_and_config32(HOST_BRIDGE, DEVEN, ~(DEVEN_D2F0 | DEVEN_D2F1));
>                     break;
>             case 2:
>                     //enable func0 only ??? TEST
>                     pci_update_config16(HOST_BRIDGE, DEVEN, ~(DEVEN_D2F1), DEVEN_D2F0);
>                     break;
>             case 1:
>             default:
>                     //enabled both
>                     pci_or_config32(HOST_BRIDGE, DEVEN, DEVEN_D2F0 | DEVEN_D2F1);
>                     break;
>     }
>
>     sdram_initialize( ...
>
> RFC:
>
> Is this approach OK? Of course the patch will need to cover the if (0) tests and skip access to missing PCI devices. Does anybody know if GCFC (Graphics Clock Frequency Control) register (i945 datasheet, section 8.1.35, page 296) needs to be explicitly disabled when IGD PCIe access is disabled by DEVEN bits or do I need to enable IGD PCIe access with DEVEN, disable clocks in GCFC and then disable IGD PCIe access again with DEVEN bits?

Unless you need to disable cdclk and crclk (the two graphics clocks
controlled by GCFC, as I see in the document number 309219-006) for
some reason, I wouldn't worry too much about the GCFC details. Your
approach looks reasonable, I'd remove the "func0 only" case for
simplicity. I think the existing code should be able to handle this
properly.
> Similar to GCFC settings without enabled IGD PCIe access. There are many registers in affected code, which doesn't have a name/description in the i945 datasheet. Does anybody know their function? It could help to determine if they needs to be set even when IGD will be disabled.

Ah yes, the magic registers. They're not publicly documented (I think
the i945 code was initially written based on information in
confidential documents), and I'm not sure if anyone still has access
to that information.
> Best regards,
> Petr
> _______________________________________________
> coreboot mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

Best regards,
Angel