Re: [PATCH v4 1/8] platform/x86/intel: Add Primary to Sideband (P2SB) bridge support

[Lukas Wunner]

On Thu, May 05, 2022 at 07:54:49PM +0200, Andy Shevchenko wrote:
> On Thu, May 5, 2022 at 4:55 PM Lukas Wunner <lukas@xxxxxxxxx> wrote:
> > On Mon, Jan 31, 2022 at 05:13:39PM +0200, Andy Shevchenko wrote:
> > > Background information
> > > ======================
> >
> > The wealth of information in the commit message obscures what the
> > actual problem is, which is actually quite simple:  SoC features
> > such as GPIO are accessed via a reserved MMIO area, we don't know
> > its address but can obtain it from the BAR of the P2SB device,
> > that device is normally hidden so we have to temporarily unhide it.
> 
> Right, but this long commit message was a result of the previous
> discussions with Bjorn. If we're ever going to handle something like
> this in the PCI core, perhaps he won't be happy if I remove it. Maybe
> we can simply state what you wrote as a problem statement and move
> this chapter at the end?

Yes, feel free to copy-paste the synopsis from my e-mail above
and rephrase as you see fit.


> > > On top of that in some cases P2SB is represented by function 0 on PCI
> > > slot (in terms of B:D.F) and according to the PCI specification any
> > > other function can't be seen until function 0 is present and visible.
> >
> > I find that paragraph confusing:  Do multi-function P2SB devices exist?
> > What are the other functions?  Are they visible but merely not enumerated
> > because function 0 is not visible?
> 
> The case I see is when we want to read the BAR from another slot of a
> PCI device, 0 function of which is P2SB. Since P2SB is hidden, the
> other device is hidden as well. Any idea how to reformulate this? And
> yes, we have this in the existing SoCs.

The spec you linked to in the commit message (for the 100 series chipset)
says that P2SB is located at Device 31 Function 1.

In those chipsets where P2SB is function 0, what kind of devices are
at functions 1 and higher?


> > Do you really need all the complicated logic in __pci_bus_read_base()?
> > For comparison, simatic_ipc_get_membase0() in simatic-ipc.c merely does:
> >
> >         pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
> >
> > If that's sufficient for simatic-ipc.c, why is the more complicated code
> > necessary in p2sb.c?
> 
> Since it's a PCI device I want to follow what PCI core does with it.
> As I explained somewhere that the current code (actually it's a
> simplified version of what is done in PCI core) follows what spec
> requires. I would like to be in alignment with the spec, while it
> still may work with less code. Besides that, it's theoretically
> possible that the base address may be 64-bit in new SoCs, I won't
> rewrite code again just because we abused the spec.

So as an alternative to copy-pasting __pci_bus_read_base(),
you could just call pci_scan_single_device().  This will create
a proper pci_dev that you can work with.  Note that no driver will
be bound to the device because of:

  pci_scan_single_device()
    pci_device_add()
      dev->match_driver = false

After you've read the BAR, get rid of the pci_dev with pci_destroy_dev().


> > > +     /*
> > > +      * I don't know how l can have all bits set.  Copied from old code.
> > > +      * Maybe it fixes a bug on some ancient platform.
> > > +      */
> > > +     if (PCI_POSSIBLE_ERROR(l))
> > > +             l = 0;
> >
> > l can have all bits set if the device was hot-removed.  That can't happen
> > with a built-in device such as P2SB.
> 
> Can be dropped, indeed. But that chicken bit emulates that :-) Anyway,
> we unhide the device before looking into it, so we shouldn't have the
> surprise "removals".

pci_lock_rescan_remove() prevents concurrent unhiding as well as
removal via sysfs.

Thanks,

Lukas