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

[Andy Shevchenko]

On Thu, Jun 10, 2021 at 4:48 PM Henning Schild
<henning.schild@xxxxxxxxxxx> wrote:
>
> Am Thu, 10 Jun 2021 13:14:49 +0300
> schrieb Andy Shevchenko <andy.shevchenko@xxxxxxxxx>:
>
> > On Thu, Jun 10, 2021 at 12:14 PM Henning Schild
> > <henning.schild@xxxxxxxxxxx> wrote:
> > >
> > > Am Mon, 8 Mar 2021 14:20:13 +0200
> > > schrieb Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>:
> > >
> > > > 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