Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1304595 > unrolled thread

Re: [PATCH V2 22/23] pci, acpi: Match PCI config space accessors against platfrom specific quirks.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V2 22/23] pci, acpi: Match PCI config space accessors  against platfrom specific quirks. Mark Salter <msalter@redhat.com> - 2016-01-08 15:20 +0100
    Re: [PATCH V2 22/23] pci, acpi: Match PCI config space accessors  against platfrom specific quirks. Tomasz Nowicki <tn@semihalf.com> - 2016-01-08 15:40 +0100
      Re: [PATCH V2 22/23] pci, acpi: Match PCI config space accessors  against platfrom specific quirks. Mark Salter <msalter@redhat.com> - 2016-01-08 16:00 +0100
    Re: [PATCH V2 22/23] pci, acpi: Match PCI config space accessors  against platfrom specific quirks. Mark Rutland <mark.rutland@arm.com> - 2016-01-08 16:10 +0100
      Re: [PATCH V2 22/23] pci, acpi: Match PCI config space accessors  against platfrom specific quirks. Mark Rutland <mark.rutland@arm.com> - 2016-01-08 16:20 +0100
        Re: [PATCH V2 22/23] pci, acpi: Match PCI config space accessors  against platfrom specific quirks. Mark Salter <msalter@redhat.com> - 2016-01-08 17:10 +0100

#1304595 — Re: [PATCH V2 22/23] pci, acpi: Match PCI config space accessors against platfrom specific quirks.

On Wed, 2015-12-16 at 16:16 +0100, Tomasz Nowicki wrote:
> Some platforms may not be fully compliant with generic set of PCI config
> accessors. For these cases we implement the way to overwrite accessors
> set before PCI buses enumeration. Algorithm that overwrite accessors
> matches against platform ID (DMI), domain and bus number, hopefully
> enough for all cases. All quirks can be defined using:
> DECLARE_ACPI_MCFG_FIXUP() and keep self contained.
> 
> example:
> 
> static const struct dmi_system_id yyy[] = {
>         {
>                 .ident = "<Platform ident string>",
>                 .callback = <handler>,
>                 .matches = {
>                         DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
>                         DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
>                         DMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
>                 },
>         },
>         { }
> };
> 

This seems awkward to me in the case where the quirk is SoC-based and there
may be multiple platforms affected. Needing a DECLARE_ACPI_MCFG_FIXUP for
each platform using such a SoC (i.e. Mustang and Moonshot) doesn't seem
right. In that case, I think it'd be better to check CPUID and possibly
some SoC register to cover all platforms affected.

Also, there doesn't seem to be a way to connect a given quirk check to the
MCFG/device requesting the ops. So if there is a platform with multiple PCIE
roots and not all of them have quirks, how does one no whether to override the
default ecam ops?

[toc] | [next] | [standalone]

#1304619

On 08.01.2016 15:16, Mark Salter wrote:
> On Wed, 2015-12-16 at 16:16 +0100, Tomasz Nowicki wrote:
>> Some platforms may not be fully compliant with generic set of PCI config
>> accessors. For these cases we implement the way to overwrite accessors
>> set before PCI buses enumeration. Algorithm that overwrite accessors
>> matches against platform ID (DMI), domain and bus number, hopefully
>> enough for all cases. All quirks can be defined using:
>> DECLARE_ACPI_MCFG_FIXUP() and keep self contained.
>>
>> example:
>>
>> static const struct dmi_system_id yyy[] = {
>>          {
>>                  .ident = "<Platform ident string>",
>>                  .callback = <handler>,
>>                  .matches = {
>>                          DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
>>                          DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
>>                          DMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
>>                  },
>>          },
>>          { }
>> };
>>
>
> This seems awkward to me in the case where the quirk is SoC-based and there
> may be multiple platforms affected. Needing a DECLARE_ACPI_MCFG_FIXUP for
> each platform using such a SoC (i.e. Mustang and Moonshot) doesn't seem
> right. In that case, I think it'd be better to check CPUID and possibly
> some SoC register to cover all platforms affected.

Right, my next version already has alternative to DMI match handler, so 
there will be two ways to match:
1. DMI, like in this patch set
2. int (*match)(struct pci_mcfg_fixup *) where you can read CPUID, and 
whatever is necessary.

>
> Also, there doesn't seem to be a way to connect a given quirk check to the
> MCFG/device requesting the ops. So if there is a platform with multiple PCIE
> roots and not all of them have quirks, how does one no whether to override the
> default ecam ops?
>

Then we can identify them using <domain:bus>. I was wondering to pass 
acpi device handler to match handler for the case where we need e.g. 
extra properties from related DSDT device descriptor. Does it make sense 
to you?

Tomasz

[toc] | [prev] | [next] | [standalone]

#1304623

On Fri, 2016-01-08 at 15:36 +0100, Tomasz Nowicki wrote:
> On 08.01.2016 15:16, Mark Salter wrote:
> > On Wed, 2015-12-16 at 16:16 +0100, Tomasz Nowicki wrote:
> > > Some platforms may not be fully compliant with generic set of PCI config
> > > accessors. For these cases we implement the way to overwrite accessors
> > > set before PCI buses enumeration. Algorithm that overwrite accessors
> > > matches against platform ID (DMI), domain and bus number, hopefully
> > > enough for all cases. All quirks can be defined using:
> > > DECLARE_ACPI_MCFG_FIXUP() and keep self contained.
> > > 
> > > example:
> > > 
> > > static const struct dmi_system_id yyy[] = {
> > >          {
> > >                  .ident = "<Platform ident string>",
> > >                  .callback = <handler>,
> > >                  .matches = {
> > >                          DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
> > >                          DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
> > >                          DMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
> > >                  },
> > >          },
> > >          { }
> > > };
> > > 
> > 
> > This seems awkward to me in the case where the quirk is SoC-based and there
> > may be multiple platforms affected. Needing a DECLARE_ACPI_MCFG_FIXUP for
> > each platform using such a SoC (i.e. Mustang and Moonshot) doesn't seem
> > right. In that case, I think it'd be better to check CPUID and possibly
> > some SoC register to cover all platforms affected.
> 
> Right, my next version already has alternative to DMI match handler, so 
> there will be two ways to match:
> 1. DMI, like in this patch set
> 2. int (*match)(struct pci_mcfg_fixup *) where you can read CPUID, and 
> whatever is necessary.
> 

Great. Thanks.

> > 
> > Also, there doesn't seem to be a way to connect a given quirk check to the
> > MCFG/device requesting the ops. So if there is a platform with multiple PCIE
> > roots and not all of them have quirks, how does one no whether to override the
> > default ecam ops?
> > 
> 
> Then we can identify them using <domain:bus>. I was wondering to pass 
> acpi device handler to match handler for the case where we need e.g. 
> extra properties from related DSDT device descriptor. Does it make sense 
> to you?
> 

The thing with domain:bus is that it is really a firmware setting. So on one
platform domain 0 may be associated with hwdev X but on another platform it
may be associated with hwdev Y. But yes, an acpi handle would make it much
easier for the match handler to find out which hw dev it is being asked to
match.

[toc] | [prev] | [next] | [standalone]

#1304638

On Fri, Jan 08, 2016 at 09:16:21AM -0500, Mark Salter wrote:
> On Wed, 2015-12-16 at 16:16 +0100, Tomasz Nowicki wrote:
> > Some platforms may not be fully compliant with generic set of PCI config
> > accessors. For these cases we implement the way to overwrite accessors
> > set before PCI buses enumeration. Algorithm that overwrite accessors
> > matches against platform ID (DMI), domain and bus number, hopefully
> > enough for all cases. All quirks can be defined using:
> > DECLARE_ACPI_MCFG_FIXUP() and keep self contained.
> > 
> > example:
> > 
> > static const struct dmi_system_id yyy[] = {
> >         {
> >                 .ident = "<Platform ident string>",
> >                 .callback = <handler>,
> >                 .matches = {
> >                         DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
> >                         DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
> >                         DMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
> >                 },
> >         },
> >         { }
> > };
> > 
> 
> This seems awkward to me in the case where the quirk is SoC-based and there
> may be multiple platforms affected. Needing a DECLARE_ACPI_MCFG_FIXUP for
> each platform using such a SoC (i.e. Mustang and Moonshot) doesn't seem
> right. In that case, I think it'd be better to check CPUID and possibly
> some SoC register to cover all platforms affected.

CPUs get reused across SoCs, so as you've implicitly noted, the CPUID
alone is insufficient.

Given that IP blocks get moved around between SoC variants, I don't
think you can check "some SoC register" based on the CPU ID -- you can
end up bringing the board down at that point.

If the CPU ID alone is insufficient to tell you about a component, it
cannot give you enough information about a component you can use to
query more information from.

If your platform requires a quirk, it's always going to be painful (and
to some extent, rightfulyl so). We should aim for correctness here with
explicit matching.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1304645

On Fri, Jan 08, 2016 at 03:01:37PM +0000, Mark Rutland wrote:
> On Fri, Jan 08, 2016 at 09:16:21AM -0500, Mark Salter wrote:
> > On Wed, 2015-12-16 at 16:16 +0100, Tomasz Nowicki wrote:
> > > Some platforms may not be fully compliant with generic set of PCI config
> > > accessors. For these cases we implement the way to overwrite accessors
> > > set before PCI buses enumeration. Algorithm that overwrite accessors
> > > matches against platform ID (DMI), domain and bus number, hopefully
> > > enough for all cases. All quirks can be defined using:
> > > DECLARE_ACPI_MCFG_FIXUP() and keep self contained.
> > > 
> > > example:
> > > 
> > > static const struct dmi_system_id yyy[] = {
> > >         {
> > >                 .ident = "<Platform ident string>",
> > >                 .callback = <handler>,
> > >                 .matches = {
> > >                         DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
> > >                         DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
> > >                         DMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
> > >                 },
> > >         },
> > >         { }
> > > };
> > > 
> > 
> > This seems awkward to me in the case where the quirk is SoC-based and there
> > may be multiple platforms affected. Needing a DECLARE_ACPI_MCFG_FIXUP for
> > each platform using such a SoC (i.e. Mustang and Moonshot) doesn't seem
> > right. In that case, I think it'd be better to check CPUID and possibly
> > some SoC register to cover all platforms affected.
> 
> CPUs get reused across SoCs, so as you've implicitly noted, the CPUID
> alone is insufficient.
> 
> Given that IP blocks get moved around between SoC variants, I don't
> think you can check "some SoC register" based on the CPU ID -- you can
> end up bringing the board down at that point.
> 
> If the CPU ID alone is insufficient to tell you about a component, it
> cannot give you enough information about a component you can use to
> query more information from.
> 
> If your platform requires a quirk, it's always going to be painful (and
> to some extent, rightfulyl so). We should aim for correctness here with
> explicit matching.

Further, if there is going to be an ever-expanding set of platforms
requring quirks, then we need a standard mechanism in ACPI to enable the
platform to tell us explicitly either which specific PCI implementation
is used, or which common quirk is necessary.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1304695

On Fri, 2016-01-08 at 15:12 +0000, Mark Rutland wrote:
> On Fri, Jan 08, 2016 at 03:01:37PM +0000, Mark Rutland wrote:
> > On Fri, Jan 08, 2016 at 09:16:21AM -0500, Mark Salter wrote:
> > > On Wed, 2015-12-16 at 16:16 +0100, Tomasz Nowicki wrote:
> > > > Some platforms may not be fully compliant with generic set of PCI config
> > > > accessors. For these cases we implement the way to overwrite accessors
> > > > set before PCI buses enumeration. Algorithm that overwrite accessors
> > > > matches against platform ID (DMI), domain and bus number, hopefully
> > > > enough for all cases. All quirks can be defined using:
> > > > DECLARE_ACPI_MCFG_FIXUP() and keep self contained.
> > > > 
> > > > example:
> > > > 
> > > > static const struct dmi_system_id yyy[] = {
> > > >         {
> > > >                 .ident = "<Platform ident string>",
> > > >                 .callback = <handler>,
> > > >                 .matches = {
> > > >                         DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
> > > >                         DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
> > > >                         DMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
> > > >                 },
> > > >         },
> > > >         { }
> > > > };
> > > > 
> > > 
> > > This seems awkward to me in the case where the quirk is SoC-based and there
> > > may be multiple platforms affected. Needing a DECLARE_ACPI_MCFG_FIXUP for
> > > each platform using such a SoC (i.e. Mustang and Moonshot) doesn't seem
> > > right. In that case, I think it'd be better to check CPUID and possibly
> > > some SoC register to cover all platforms affected.
> > 
> > CPUs get reused across SoCs, so as you've implicitly noted, the CPUID
> > alone is insufficient.
> > 
> > Given that IP blocks get moved around between SoC variants, I don't
> > think you can check "some SoC register" based on the CPU ID -- you can
> > end up bringing the board down at that point.
> > 
> > If the CPU ID alone is insufficient to tell you about a component, it
> > cannot give you enough information about a component you can use to
> > query more information from.
> > 
> > If your platform requires a quirk, it's always going to be painful (and
> > to some extent, rightfulyl so). We should aim for correctness here with
> > explicit matching.
> 
> Further, if there is going to be an ever-expanding set of platforms
> requring quirks, then we need a standard mechanism in ACPI to enable the
> platform to tell us explicitly either which specific PCI implementation
> is used, or which common quirk is necessary.
> 

No, an ever-expanding set is exactly what we don't want. I think you've convinced
me that I'm taking a wrong view of the problem. Putting something in the ACPI
standard would be going too far and I think a hard sell to the standards folk.
There really is no foolproof way to match a plug and play ACPI PCIe root to
specific hardware without considering the exact platform and/or BIOS info. So
yeah, it should be painful in order to give incentive to the silicon vendors to
get it right.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web