| Message ID | 20210308122020.57071-1-andriy.shevchenko@linux.intel.com |
|---|---|
| Headers | show |
| Series | PCI: introduce p2sb helper | expand |
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 >
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 >
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). -- With Best Regards, Andy Shevchenko
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). > >
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). -- With Best Regards, Andy Shevchenko
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.
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