Change in coreboot[master]: soc/intel/common: Expand the Primary to Sideband bridge

[Subrata Banik (Code Review)]

Attention is currently required from: Wonkyu Kim, John Zhao, Tim Wawrzynczak, Angel Pons, Nick Vaccaro, Patrick Rudolph.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61283 )

Change subject: soc/intel/common: Expand the Primary to Sideband bridge
......................................................................

Patch Set 3:

(1 comment)

Patchset:

PS3:
> It might be valid to add P2SB_32_BIT_BAR to all platforms and update like "if ((dev == PCH_DEV_P2SB) && CONFIG(P2SB_32_BIT_BAR))".

We could do that as well but won't that be like violating the EDS where says P2SB (PCH) for all current and previous generation has 64-bit BAR.
> If considering to put both P2SB/P2SB2 into the p2sb.c, how can the mmio_resource be handled in the read_resource function with different BAR address?

mmio_resource(dev, 0, P2SB2_BAR / KiB, P2SB_SIZE / KiB);
vs
mmio_resource(dev, 0, P2SB2_BAR / KiB, P2SB2_SIZE / KiB);

We can have our custom PCI read/set mmio function that internally check the DEV PCI B:D:F and allocate the resource.
> Based on the "dev" to refer to different P2SB BAR while referring to mmio_resource?

yes.
> As mentioned, there are common features across p2sb and p2sb2 like the P2SB generic configuration registers offset, etc. But there also exist difference, i.e lock(mask EPs and hide) function in which the masking EPs only appears valid and executed towards p2sb(you might probe the fsp code execution). p2sb and p2sb2 have different addressing regions(32 bit vs 64 bit) and size. It seems proper and clear to have a p2sb_lib.c which has the real common code like hide/unhide/enable_bar. p2sb and p2sb2 hold their own framework and dedicated functions (like hpet/apic for p2sb.c, etc) as treated different pci devices.

Sure, why don't you start with creating a p2sb_lib as you have mentioned here and at outer layer to two files as p2sb.c and p2sb2 or p2sb_ioe.c
> Can you consider to change the term "SOC" usage in CB:61297?

I don't really mean to make CB:61297 landed into the master. Do you think its reasonable for you to develop p2sb2/ioe p2sb using CB:61297 else you can ignore that CL. My intention was to design something that represents the SOC internal saying PCH/SoC or IOE.
> p2sb is referred to PCH besides the CPU die. MTL has the additional IOE die with p2sb2. As you might recall, future platforms besides the MTL drop the IOE with different architecture, like computerX die, ChipsetX and PCH into SoC die, etc. It seems aligned to replace the "SOC" with "PCH". We will expand and have PCH/IOE for MTL and PCH/ChipsetX for future platforms.

But as you said, IOE may not be scalable then we might need to keep on adding new interface besides PCH for each SOC platform. Hence, I have kept the p2sb_type enum inside SOC directory.

if you want, I can change below macro from SOC to PCH.

enum p2sb_types {
    PCH = PCI_DEV(0, PCH_DEV_SLOT_ESPI, 1),
};

for MTL it might be like

enum p2sb_types {
    PCH = PCI_DEV(0, PCH_DEV_SLOT_ESPI, 1),
        IOE = PCI_DEV(0, 0x19, 0),
};

for more distance future

enum p2sb_types {
    PCH = PCI_DEV(0, PCH_DEV_SLOT_ESPI, 1),
        CHIPSETX = PCI_DEV(0, 0x19, 0),
};

WDYT?