Date:   Sun, 8 May 2022 12:05:53 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     Wolfram Sang <wsa@...nel.org>, Jean Delvare <jdelvare@...e.de>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Lee Jones <lee.jones@...aro.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Tan Jui Nee <jui.nee.tan@...el.com>,
        Kate Hsuan <hpa@...hat.com>,
        Jonathan Yong <jonathan.yong@...el.com>,
        linux-kernel@...r.kernel.org, linux-edac@...r.kernel.org,
        linux-i2c@...r.kernel.org, linux-gpio@...r.kernel.org,
        platform-driver-x86@...r.kernel.org,
        Borislav Petkov <bp@...en8.de>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Tony Luck <tony.luck@...el.com>,
        James Morse <james.morse@....com>,
        Robert Richter <rric@...nel.org>,
        Peter Tyser <ptyser@...-inc.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Mark Gross <markgross@...nel.org>,
        Henning Schild <henning.schild@...mens.com>
Subject: Re: [PATCH v4 1/8] platform/x86/intel: Add Primary to Sideband (P2SB)
 bridge support

On Sun, May 8, 2022 at 9:13 AM Lukas Wunner <lukas@...ner.de> wrote:
> 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@...ner.de> 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.

Will do.

...

> > > > 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?

In the Intel Broxton and Apollo Lake cases the P2SB is the function 0
and we want to have a BAR of SPI NOR, which is function 2.

...

> > > 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().

This is pretty nice, if it flies! I definitely try this ASAP (during
working hours).
Thanks for the hint.

...

> > > > +     /*
> > > > +      * 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.

Yep, that's good. In any case this piece of code will be gone if your
above suggestion works, have I got it right?

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists