mbox series

[rfc,v1,0/7] PCI: introduce p2sb helper

Message ID 20210308122020.57071-1-andriy.shevchenko@linux.intel.com (mailing list archive)
Headers show
Series PCI: introduce p2sb helper | expand

Message

Andy Shevchenko March 8, 2021, 12:20 p.m. UTC
There are a few users and even at least one more is coming
that would like to utilize p2sb mechanisms like hide/unhide
a device from PCI configuration space.

Here is the series to deduplicate existing users and provide
a generic way for new comers.

It also includes a patch to enable GPIO controllers on Apollo Lake
when it's used with ABL bootloader w/o ACPI support.

Please, comment on the approach and individual patches.

(Since it's cross subsystem, the PCI seems like a main one and
 I think it makes sense to route it thru it with immutable tag
 or branch provided for the others).

Andy Shevchenko (5):
  PCI: Introduce pci_bus_*() printing macros when device is not
    available
  PCI: Convert __pci_read_base() to __pci_bus_read_base()
  mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()
  mfd: lpc_ich: Switch to generic pci_p2sb_bar()
  i2c: i801: convert to use common P2SB accessor

Jonathan Yong (1):
  PCI: New Primary to Sideband (P2SB) bridge support library

Tan Jui Nee (1):
  mfd: lpc_ich: Add support for pinctrl in non-ACPI system

 drivers/i2c/busses/Kconfig    |   1 +
 drivers/i2c/busses/i2c-i801.c |  40 +++-------
 drivers/mfd/Kconfig           |   1 +
 drivers/mfd/lpc_ich.c         | 135 +++++++++++++++++++++++++++++-----
 drivers/pci/Kconfig           |   8 ++
 drivers/pci/Makefile          |   1 +
 drivers/pci/pci-p2sb.c        |  89 ++++++++++++++++++++++
 drivers/pci/pci.h             |  13 +++-
 drivers/pci/probe.c           |  81 ++++++++++----------
 include/linux/pci-p2sb.h      |  28 +++++++
 include/linux/pci.h           |   9 +++
 11 files changed, 313 insertions(+), 93 deletions(-)
 create mode 100644 drivers/pci/pci-p2sb.c
 create mode 100644 include/linux/pci-p2sb.h

Comments

Henning Schild March 13, 2021, 9:25 a.m. UTC | #1
Am Mon, 8 Mar 2021 14:20:13 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> There are a few users and even at least one more is coming
> that would like to utilize p2sb mechanisms like hide/unhide
> a device from PCI configuration space.

Tried this for my usecase and can confirm it to work as expected.

Tested-by: Henning Schild <henning.schild@siemens.com>

Henning

> Here is the series to deduplicate existing users and provide
> a generic way for new comers.
> 
> It also includes a patch to enable GPIO controllers on Apollo Lake
> when it's used with ABL bootloader w/o ACPI support.
> 
> Please, comment on the approach and individual patches.
> 
> (Since it's cross subsystem, the PCI seems like a main one and
>  I think it makes sense to route it thru it with immutable tag
>  or branch provided for the others).
> 
> Andy Shevchenko (5):
>   PCI: Introduce pci_bus_*() printing macros when device is not
>     available
>   PCI: Convert __pci_read_base() to __pci_bus_read_base()
>   mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()
>   mfd: lpc_ich: Switch to generic pci_p2sb_bar()
>   i2c: i801: convert to use common P2SB accessor
> 
> Jonathan Yong (1):
>   PCI: New Primary to Sideband (P2SB) bridge support library
> 
> Tan Jui Nee (1):
>   mfd: lpc_ich: Add support for pinctrl in non-ACPI system
> 
>  drivers/i2c/busses/Kconfig    |   1 +
>  drivers/i2c/busses/i2c-i801.c |  40 +++-------
>  drivers/mfd/Kconfig           |   1 +
>  drivers/mfd/lpc_ich.c         | 135
> +++++++++++++++++++++++++++++----- drivers/pci/Kconfig           |
> 8 ++ drivers/pci/Makefile          |   1 +
>  drivers/pci/pci-p2sb.c        |  89 ++++++++++++++++++++++
>  drivers/pci/pci.h             |  13 +++-
>  drivers/pci/probe.c           |  81 ++++++++++----------
>  include/linux/pci-p2sb.h      |  28 +++++++
>  include/linux/pci.h           |   9 +++
>  11 files changed, 313 insertions(+), 93 deletions(-)
>  create mode 100644 drivers/pci/pci-p2sb.c
>  create mode 100644 include/linux/pci-p2sb.h
>
Henning Schild June 10, 2021, 9:02 a.m. UTC | #2
Am Mon, 8 Mar 2021 14:20:13 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> There are a few users and even at least one more is coming
> that would like to utilize p2sb mechanisms like hide/unhide
> a device from PCI configuration space.
> 
> Here is the series to deduplicate existing users and provide
> a generic way for new comers.
> 
> It also includes a patch to enable GPIO controllers on Apollo Lake
> when it's used with ABL bootloader w/o ACPI support.

That bit is especially interesting. Making pinctl*lake initialize when
ACPI IDs are missing and p2sb is hidden.

However i have seen pinctl-broxton get confused because it was trying
to come up twice on a system that has the ACPI entries. Once as
"INT3452" and second as "apollolake-pinctrl". They should probably
mutually exclude each other. And the two different names for "the
same"? thing make it impossible to write a driver using those GPIOs.
Unless it would try and look up both variants or not looking up with
gpiochip.label.

I would also need that "enable GPIO w/o ACPI" for skylake. I think it
would be generally useful if the GPIO controllers would be enabled not
depending on ACPI, and coming up with only one "label" to build on top.

regards,
Henning

> Please, comment on the approach and individual patches.
> 
> (Since it's cross subsystem, the PCI seems like a main one and
>  I think it makes sense to route it thru it with immutable tag
>  or branch provided for the others).
> 
> Andy Shevchenko (5):
>   PCI: Introduce pci_bus_*() printing macros when device is not
>     available
>   PCI: Convert __pci_read_base() to __pci_bus_read_base()
>   mfd: lpc_ich: Factor out lpc_ich_enable_spi_write()
>   mfd: lpc_ich: Switch to generic pci_p2sb_bar()
>   i2c: i801: convert to use common P2SB accessor
> 
> Jonathan Yong (1):
>   PCI: New Primary to Sideband (P2SB) bridge support library
> 
> Tan Jui Nee (1):
>   mfd: lpc_ich: Add support for pinctrl in non-ACPI system
> 
>  drivers/i2c/busses/Kconfig    |   1 +
>  drivers/i2c/busses/i2c-i801.c |  40 +++-------
>  drivers/mfd/Kconfig           |   1 +
>  drivers/mfd/lpc_ich.c         | 135
> +++++++++++++++++++++++++++++----- drivers/pci/Kconfig           |
> 8 ++ drivers/pci/Makefile          |   1 +
>  drivers/pci/pci-p2sb.c        |  89 ++++++++++++++++++++++
>  drivers/pci/pci.h             |  13 +++-
>  drivers/pci/probe.c           |  81 ++++++++++----------
>  include/linux/pci-p2sb.h      |  28 +++++++
>  include/linux/pci.h           |   9 +++
>  11 files changed, 313 insertions(+), 93 deletions(-)
>  create mode 100644 drivers/pci/pci-p2sb.c
>  create mode 100644 include/linux/pci-p2sb.h
>
Andy Shevchenko June 10, 2021, 10:14 a.m. UTC | #3
On Thu, Jun 10, 2021 at 12:14 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> Am Mon, 8 Mar 2021 14:20:13 +0200
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
>
> > There are a few users and even at least one more is coming
> > that would like to utilize p2sb mechanisms like hide/unhide
> > a device from PCI configuration space.
> >
> > Here is the series to deduplicate existing users and provide
> > a generic way for new comers.
> >
> > It also includes a patch to enable GPIO controllers on Apollo Lake
> > when it's used with ABL bootloader w/o ACPI support.
>
> That bit is especially interesting. Making pinctl*lake initialize when
> ACPI IDs are missing and p2sb is hidden.
>
> However i have seen pinctl-broxton get confused because it was trying
> to come up twice on a system that has the ACPI entries. Once as
> "INT3452" and second as "apollolake-pinctrl". They should probably
> mutually exclude each other. And the two different names for "the
> same"? thing make it impossible to write a driver using those GPIOs.

Then it's clearly told that BIOS provides confusing data, it exposes
the ACPI device and hides it in p2sb, how is it even supposed to work?

I consider only these are valid:
 - ACPI device is provided and it's enabled (status = 15) => work with
ACPI enumeration
 - no ACPI device provided and it's hidden or not by p2sb => work via board file
 - no ACPI device provided and no device needed / present => no driver is needed

> Unless it would try and look up both variants or not looking up with
> gpiochip.label.
>
> I would also need that "enable GPIO w/o ACPI" for skylake.

Not a problem to add a platform driver name there or actually for all
of the Intel pin control drivers (depends what suits better to the
current design).

>  I think it
> would be generally useful if the GPIO controllers would be enabled not
> depending on ACPI, and coming up with only one "label" to build on top.

I didn't get what 'label' means here...

> > Please, comment on the approach and individual patches.
> >
> > (Since it's cross subsystem, the PCI seems like a main one and
> >  I think it makes sense to route it thru it with immutable tag
> >  or branch provided for the others).
Henning Schild June 10, 2021, 1:48 p.m. UTC | #4
Am Thu, 10 Jun 2021 13:14:49 +0300
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Thu, Jun 10, 2021 at 12:14 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > Am Mon, 8 Mar 2021 14:20:13 +0200
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> >  
> > > There are a few users and even at least one more is coming
> > > that would like to utilize p2sb mechanisms like hide/unhide
> > > a device from PCI configuration space.
> > >
> > > Here is the series to deduplicate existing users and provide
> > > a generic way for new comers.
> > >
> > > It also includes a patch to enable GPIO controllers on Apollo Lake
> > > when it's used with ABL bootloader w/o ACPI support.  
> >
> > That bit is especially interesting. Making pinctl*lake initialize
> > when ACPI IDs are missing and p2sb is hidden.
> >
> > However i have seen pinctl-broxton get confused because it was
> > trying to come up twice on a system that has the ACPI entries. Once
> > as "INT3452" and second as "apollolake-pinctrl". They should
> > probably mutually exclude each other. And the two different names
> > for "the same"? thing make it impossible to write a driver using
> > those GPIOs.  
> 
> Then it's clearly told that BIOS provides confusing data, it exposes
> the ACPI device and hides it in p2sb, how is it even supposed to work?

The patchset works fine on a machine with hidden p2sb and no ACPI,
except for the NULL pointer issue i sent that patch for.

The problem appeared with the patchset being used on a machine having
ACPI entries and a visible p2sb. 

> I consider only these are valid:
>  - ACPI device is provided and it's enabled (status = 15) => work with
> ACPI enumeration
>  - no ACPI device provided and it's hidden or not by p2sb => work via
> board file
>  - no ACPI device provided and no device needed / present => no
> driver is needed
> 
> > Unless it would try and look up both variants or not looking up with
> > gpiochip.label.
> >
> > I would also need that "enable GPIO w/o ACPI" for skylake.  
> 
> Not a problem to add a platform driver name there or actually for all
> of the Intel pin control drivers (depends what suits better to the
> current design).
> 
> >  I think it
> > would be generally useful if the GPIO controllers would be enabled
> > not depending on ACPI, and coming up with only one "label" to build
> > on top.  
> 
> I didn't get what 'label' means here...

The name of the gpiochip /sys/class/gpiochipxxx/label or the first arg
to GPIO_LOOKUP_IDX
It seems to me that the very same device driver can come up as
"apollolake-pinctrl.0" or "INT3452.0" depending on ACPI table entries.

Henning

> > > Please, comment on the approach and individual patches.
> > >
> > > (Since it's cross subsystem, the PCI seems like a main one and
> > >  I think it makes sense to route it thru it with immutable tag
> > >  or branch provided for the others).  
> 
>
Andy Shevchenko June 10, 2021, 2:04 p.m. UTC | #5
On Thu, Jun 10, 2021 at 4:48 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> Am Thu, 10 Jun 2021 13:14:49 +0300
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
>
> > On Thu, Jun 10, 2021 at 12:14 PM Henning Schild
> > <henning.schild@siemens.com> wrote:
> > >
> > > Am Mon, 8 Mar 2021 14:20:13 +0200
> > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> > >
> > > > There are a few users and even at least one more is coming
> > > > that would like to utilize p2sb mechanisms like hide/unhide
> > > > a device from PCI configuration space.
> > > >
> > > > Here is the series to deduplicate existing users and provide
> > > > a generic way for new comers.
> > > >
> > > > It also includes a patch to enable GPIO controllers on Apollo Lake
> > > > when it's used with ABL bootloader w/o ACPI support.
> > >
> > > That bit is especially interesting. Making pinctl*lake initialize
> > > when ACPI IDs are missing and p2sb is hidden.
> > >
> > > However i have seen pinctl-broxton get confused because it was
> > > trying to come up twice on a system that has the ACPI entries. Once
> > > as "INT3452" and second as "apollolake-pinctrl". They should
> > > probably mutually exclude each other. And the two different names
> > > for "the same"? thing make it impossible to write a driver using
> > > those GPIOs.
> >
> > Then it's clearly told that BIOS provides confusing data, it exposes
> > the ACPI device and hides it in p2sb, how is it even supposed to work?
>
> The patchset works fine on a machine with hidden p2sb and no ACPI,
> except for the NULL pointer issue i sent that patch for.
>
> The problem appeared with the patchset being used on a machine having
> ACPI entries and a visible p2sb.

Yep, got it. So, basically we have to do something like call
acpi_dev_present() and forbid the platform device enumeration in this
case.

> > I consider only these are valid:
> >  - ACPI device is provided and it's enabled (status = 15) => work with
> > ACPI enumeration
> >  - no ACPI device provided and it's hidden or not by p2sb => work via
> > board file
> >  - no ACPI device provided and no device needed / present => no
> > driver is needed
> >
> > > Unless it would try and look up both variants or not looking up with
> > > gpiochip.label.
> > >
> > > I would also need that "enable GPIO w/o ACPI" for skylake.
> >
> > Not a problem to add a platform driver name there or actually for all
> > of the Intel pin control drivers (depends what suits better to the
> > current design).
> >
> > >  I think it
> > > would be generally useful if the GPIO controllers would be enabled
> > > not depending on ACPI, and coming up with only one "label" to build
> > > on top.
> >
> > I didn't get what 'label' means here...
>
> The name of the gpiochip /sys/class/gpiochipxxx/label or the first arg
> to GPIO_LOOKUP_IDX
> It seems to me that the very same device driver can come up as
> "apollolake-pinctrl.0" or "INT3452.0" depending on ACPI table entries.

Which is not a problem. Or is it? The proper way is to use character
devices and find the controller based on other means than the device
instance name, but user space also can cope with these two, Since we
never had a platform that did it in the upstream there is no formal
ABI breakage or so.

> > > > Please, comment on the approach and individual patches.
> > > >
> > > > (Since it's cross subsystem, the PCI seems like a main one and
> > > >  I think it makes sense to route it thru it with immutable tag
> > > >  or branch provided for the others).
Andy Shevchenko Nov. 26, 2021, 3:43 p.m. UTC | #6
On Mon, Mar 08, 2021 at 02:20:13PM +0200, Andy Shevchenko wrote:
> There are a few users and even at least one more is coming
> that would like to utilize p2sb mechanisms like hide/unhide
> a device from PCI configuration space.
> 
> Here is the series to deduplicate existing users and provide
> a generic way for new comers.
> 
> It also includes a patch to enable GPIO controllers on Apollo Lake
> when it's used with ABL bootloader w/o ACPI support.
> 
> Please, comment on the approach and individual patches.
> 
> (Since it's cross subsystem, the PCI seems like a main one and
>  I think it makes sense to route it thru it with immutable tag
>  or branch provided for the others).

TWIMC, after refreshing (a bit) my memories on this thread, I think the roadmap
may look like the following:

1) exporting necessary APIs from PCI core to avoid code dup;
2) moving pci-p2sb.c out from PCI to PDx86 where it seems naturally fit;
3) addressing comments on patches that are not going to change their location /
functionality;
4) adding tags, etc.

Any objections?

Meanwhile I will try to setup a machine with ACPI tables to test the code if
they have not been provided.