Linux-i2c Archive on lore.kernel.org
 help / color / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>,
	Jean Delvare <jdelvare@suse.de>, Lee Jones <lee.jones@linaro.org>,
	Tan Jui Nee <jui.nee.tan@intel.com>,
	Jim Quinlan <james.quinlan@broadcom.com>,
	Jonathan Yong <jonathan.yong@intel.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org,
	linux-pci@vger.kernel.org, Jean Delvare <jdelvare@suse.com>,
	Peter Tyser <ptyser@xes-inc.com>,
	hdegoede@redhat.com, henning.schild@siemens.com
Subject: Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
Date: Mon, 8 Mar 2021 19:42:21 -0600
Message-ID: <20210309014221.GA1831206@bjorn-Precision-5520> (raw)
In-Reply-To: <YEZ4IitUa+I9HM5F@smile.fi.intel.com>

On Mon, Mar 08, 2021 at 09:16:50PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 08, 2021 at 12:52:12PM -0600, Bjorn Helgaas wrote:
> > On Mon, Mar 08, 2021 at 02:20:16PM +0200, Andy Shevchenko wrote:
> > > From: Jonathan Yong <jonathan.yong@intel.com>
> > > 
> > > There is already one and at least one more user is coming which
> > > requires an access to Primary to Sideband bridge (P2SB) in order to
> > > get IO or MMIO bar hidden by BIOS. Create a library to access P2SB
> > > for x86 devices.
> > 
> > Can you include a spec reference?
> 
> I'm not sure I have a public link to the spec. It's the 100 Series PCH [1].
> The document number to look for is 546955 [2] and there actually a bit of
> information about this.

This link, found by googling for "p2sb bridge", looks like it might
have relevant public links:

https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/

I'd prefer if you could dig out the relevant sections because I really
don't know how to identify them.

> > I'm trying to figure out why this
> > belongs in drivers/pci/.  It looks very device-specific.
> 
> Because it's all about access to PCI configuration spaces of the (hidden)
> devices.

The PCI core generally doesn't deal with device-specific config
registers.

> [1]: https://ark.intel.com/content/www/us/en/ark/products/series/98456/intel-100-series-desktop-chipsets.html
> [2]: https://medium.com/@jacksonchen_43335/bios-gpio-p2sb-70e9b829b403
> 
> ...
> 
> > > +config PCI_P2SB
> > > +	bool "Primary to Sideband (P2SB) bridge access support"
> > > +	depends on PCI && X86
> > > +	help
> > > +	  The Primary to Sideband bridge is an interface to some PCI
> > > +	  devices connected through it. In particular, SPI NOR
> > > +	  controller in Intel Apollo Lake SoC is one of such devices.
> > 
> > This doesn't sound like a "bridge".  If it's a bridge, what's on the
> > primary (upstream) side?  What's on the secondary side?  What
> > resources are passed through the bridge, i.e., what transactions does
> > it transfer from one side to the other?
> 
> It's a confusion terminology here. It's a Bridge according to the spec, but
> it is *not* a PCI Bridge as you may had a first impression.

The code suggests that a register on this device controls whether a
different device is visible in config space.  I think it will be
better if we can describe what's happening.

> ...
> 
> > > +	/* Unhide the P2SB device */
> > > +	pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, 0);
> > > +
> > > +	/* Read the first BAR of the device in question */
> > > +	__pci_bus_read_base(bus, devfn, pci_bar_unknown, mem, PCI_BASE_ADDRESS_0, true);
> > 
> > I don't get this.  Apparently this normally hidden device is consuming
> > PCI address space.  The PCI core needs to know about this.  If it
> > doesn't, the PCI core may assign this space to another device.
> 
> Right, it returns all 1:s to any request so PCI core *thinks* it's
> plugged off (like D3cold or so).

I'm asking about the MMIO address space.  The BAR is a register in
config space.  AFAICT, clearing P2SBC_HIDE_BYTE makes that BAR
visible.  The BAR describes a region of PCI address space.  It looks
like setting P2SBC_HIDE_BIT makes the BAR disappear from config space,
but it sounds like the PCI address space *described* by the BAR is
still claimed by the device.  If the device didn't respond to that
MMIO space, you would have no reason to read the BAR at all.

So what keeps the PCI core from assigning that MMIO space to another
device?

This all sounds quite irregular from the point of view of the PCI
core.  If a device responds to address space that is not described by
a standard PCI BAR, or by an EA capability, or by one of the legacy
VGA or IDE exceptions, we have a problem.  That space must be
described *somehow* in a generic way, e.g., ACPI or similar.

What happens if CONFIG_PCI_P2SB is unset?  The device doesn't know
that, and if it is still consuming MMIO address space that we don't
know about, that's a problem.

> > > +	/* Hide the P2SB device */
> > > +	pci_bus_write_config_byte(bus, df, P2SBC_HIDE_BYTE, P2SBC_HIDE_BIT);
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

  reply index

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-08 12:20 [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Andy Shevchenko
2021-03-08 12:20 ` [PATCH v1 1/7] PCI: Introduce pci_bus_*() printing macros when device is not available Andy Shevchenko
2021-03-10 14:57   ` Jean Delvare
2021-03-08 12:20 ` [PATCH v1 2/7] PCI: Convert __pci_read_base() to __pci_bus_read_base() Andy Shevchenko
2021-03-10 17:14   ` Christoph Hellwig
2021-03-08 12:20 ` [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library Andy Shevchenko
2021-03-08 18:52   ` Bjorn Helgaas
2021-03-08 19:16     ` Andy Shevchenko
2021-03-09  1:42       ` Bjorn Helgaas [this message]
2021-03-09  8:42         ` Henning Schild
2021-04-01 15:45           ` Andy Shevchenko
2021-04-01 16:42             ` Bjorn Helgaas
2021-04-01 18:23               ` Andy Shevchenko
2021-04-01 18:44                 ` Bjorn Helgaas
2021-04-02 13:09           ` Enrico Weigelt, metux IT consult
2021-04-06 13:40             ` Henning Schild
2021-03-13  9:45   ` Henning Schild
2021-04-01 15:43     ` Andy Shevchenko
2021-04-01 18:06       ` Mika Westerberg
2021-04-01 18:22         ` Andy Shevchenko
2021-04-01 18:32           ` Mika Westerberg
2021-03-08 12:20 ` [PATCH v1 4/7] mfd: lpc_ich: Factor out lpc_ich_enable_spi_write() Andy Shevchenko
2021-03-10 10:29   ` Lee Jones
2021-03-08 12:20 ` [PATCH v1 5/7] mfd: lpc_ich: Switch to generic pci_p2sb_bar() Andy Shevchenko
2021-03-10 10:35   ` Lee Jones
2021-03-10 12:05     ` Andy Shevchenko
2021-03-10 12:57       ` Lee Jones
2021-03-08 12:20 ` [PATCH v1 6/7] mfd: lpc_ich: Add support for pinctrl in non-ACPI system Andy Shevchenko
2021-03-10 10:27   ` Lee Jones
2021-04-12 16:01   ` Henning Schild
2021-04-12 16:40     ` Henning Schild
2021-04-12 16:59       ` Andy Shevchenko
2021-04-12 17:16         ` Henning Schild
2021-04-12 17:34           ` Andy Shevchenko
2021-04-13  6:47             ` Henning Schild
2021-04-12 16:51     ` Andy Shevchenko
2021-04-12 17:27       ` Henning Schild
2021-04-12 17:41         ` Andy Shevchenko
2021-03-08 12:20 ` [PATCH v1 7/7] i2c: i801: convert to use common P2SB accessor Andy Shevchenko
2021-03-10 14:51   ` Jean Delvare
2021-03-13  9:25 ` [rfc, PATCH v1 0/7] PCI: introduce p2sb helper Henning Schild
2021-06-10  9:02 ` Henning Schild
2021-06-10 10:14   ` Andy Shevchenko
2021-06-10 13:48     ` Henning Schild
2021-06-10 14:04       ` Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210309014221.GA1831206@bjorn-Precision-5520 \
    --to=helgaas@kernel.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bhelgaas@google.com \
    --cc=hdegoede@redhat.com \
    --cc=henning.schild@siemens.com \
    --cc=james.quinlan@broadcom.com \
    --cc=jdelvare@suse.com \
    --cc=jdelvare@suse.de \
    --cc=jonathan.yong@intel.com \
    --cc=jui.nee.tan@intel.com \
    --cc=lee.jones@linaro.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=ptyser@xes-inc.com \
    --cc=wsa+renesas@sang-engineering.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-i2c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i2c/0 linux-i2c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i2c linux-i2c/ https://lore.kernel.org/linux-i2c \
		linux-i2c@vger.kernel.org
	public-inbox-index linux-i2c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-i2c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git