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

Groups > linux.kernel > #1480276 > unrolled thread

[PATCH V6 0/5] ECAM quirks handling for ARM64 platforms

Back to article view | Back to linux.kernel

Contents

  [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Tomasz Nowicki <tn@semihalf.com> - 2016-09-09 21:30 +0200
    [PATCH V6 4/5] PCI: thunder: Enable ACPI PCI controller for ThunderX pass2.x silicon version Tomasz Nowicki <tn@semihalf.com> - 2016-09-09 21:30 +0200
      Re: [PATCH V6 4/5] PCI: thunder: Enable ACPI PCI controller for  ThunderX pass2.x silicon version Bjorn Helgaas <helgaas@kernel.org> - 2016-09-19 17:50 +0200
        Re: [PATCH V6 4/5] PCI: thunder: Enable ACPI PCI controller for  ThunderX pass2.x silicon version Tomasz Nowicki <tn@semihalf.com> - 2016-09-20 09:10 +0200
          Re: [PATCH V6 4/5] PCI: thunder: Enable ACPI PCI controller for  ThunderX pass2.x silicon version Bjorn Helgaas <helgaas@kernel.org> - 2016-09-20 15:20 +0200
            Re: [PATCH V6 4/5] PCI: thunder: Enable ACPI PCI controller for  ThunderX pass2.x silicon version Tomasz Nowicki <tn@semihalf.com> - 2016-09-21 10:10 +0200
    [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case Tomasz Nowicki <tn@semihalf.com> - 2016-09-09 21:30 +0200
      Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-19 20:10 +0200
        Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Tomasz Nowicki <tn@semihalf.com> - 2016-09-20 09:30 +0200
          Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-20 15:40 +0200
            Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-09-20 15:50 +0200
              Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-20 16:10 +0200
                Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-09-20 17:10 +0200
                  Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-20 21:20 +0200
                    Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-09-21 16:10 +0200
                      Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-21 20:10 +0200
                        Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Duc Dang <dhdang@apm.com> - 2016-09-21 21:10 +0200
                          Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-21 21:20 +0200
                            Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Tomasz Nowicki <tn@semihalf.com> - 2016-09-23 13:00 +0200
                        Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-09-22 11:50 +0200
                          RE: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-22 13:20 +0200
                            Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-09-22 14:50 +0200
                              Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-22 20:40 +0200
                                Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-23 00:20 +0200
                                  Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-09-23 12:20 +0200
                                    RE: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-23 13:00 +0200
                          Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Christopher Covington <cov@codeaurora.org> - 2016-09-22 16:30 +0200
                    RE: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-21 16:20 +0200
                      Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Bjorn Helgaas <helgaas@kernel.org> - 2016-09-21 21:00 +0200
                        RE: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific  register range for ACPI case Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-22 13:20 +0200
    [PATCH V6 1/5] PCI/ACPI: Extend pci_mcfg_lookup() responsibilities Tomasz Nowicki <tn@semihalf.com> - 2016-09-09 21:30 +0200
    Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Tomasz Nowicki <tn@semihalf.com> - 2016-09-09 21:40 +0200
    Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Bjorn Helgaas <helgaas@kernel.org> - 2016-09-20 21:30 +0200
      Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms cov@codeaurora.org - 2016-09-21 03:20 +0200
        Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Bjorn Helgaas <helgaas@kernel.org> - 2016-09-21 15:20 +0200
          Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Sinan Kaya <okaya@codeaurora.org> - 2016-09-21 16:10 +0200
            Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Sinan Kaya <okaya@codeaurora.org> - 2016-09-21 19:40 +0200
            Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Bjorn Helgaas <helgaas@kernel.org> - 2016-09-21 19:40 +0200
              [PATCHv2] PCI: QDF2432 32 bit config space accessors Christopher Covington <cov@codeaurora.org> - 2016-09-22 00:40 +0200
          Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Christopher Covington <cov@codeaurora.org> - 2016-09-22 00:50 +0200
            Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms Bjorn Helgaas <helgaas@kernel.org> - 2016-09-23 01:10 +0200

Page 1 of 3  [1] 2 3  Next page →

#1480276 — [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms

Quirk handling relies on an idea of simple static array which contains
quirk enties. Each entry consists of identification information (IDs from
standard header of MCFG table) along with custom pci_ecam_ops structure and 
configuration space resource structure. This way it is possible find
corresponding quirk entries and override pci_ecam_ops and PCI configuration
space regions.

As an example, the last 3 patches present quirk handling mechanism usage for
ThunderX.

v5 -> v6
- rebase against v4.8-rc5
- drop patch 1 form previous series
- keep pci_acpi_setup_ecam_mapping() in ARM64 arch directory
- move quirk code to pci_mcfg.c
- restrict quirk to override pci_ecam_ops and CFG resource structure
  only, no init call any more
- split ThunderX quirks into the smaller chunks
- add ThunderX pass1.x silicon revision support

v4 -> v5
- rebase against v4.8-rc1
- rework to exact MCFG OEM ID, TABLE ID, rev match
  - use memcmp instead of strncmp
  - no substring match
- fix typos and dmesg message

Tomasz Nowicki (5):
  PCI/ACPI: Extend pci_mcfg_lookup() responsibilities
  PCI/ACPI: Check platform specific ECAM quirks
  PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI
    case
  PCI: thunder: Enable ACPI PCI controller for ThunderX pass2.x silicon
    version
  PCI: thunder: Enable ACPI PCI controller for ThunderX pass1.x silicon
    version

 arch/arm64/kernel/pci.c             |  17 ++--
 drivers/acpi/pci_mcfg.c             | 168 +++++++++++++++++++++++++++++++++++-
 drivers/pci/host/pci-thunder-ecam.c |   2 +-
 drivers/pci/host/pci-thunder-pem.c  |  63 +++++++++++---
 include/linux/pci-acpi.h            |   4 +-
 include/linux/pci-ecam.h            |   7 ++
 6 files changed, 230 insertions(+), 31 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1480278 — [PATCH V6 4/5] PCI: thunder: Enable ACPI PCI controller for ThunderX pass2.x silicon version

ThunderX PCIe controller to off-chip devices (so-called PEM) is not fully
compliant with ECAM standard. It uses non-standard configuration space
accessors (see pci_thunder_pem_ops) and custom configuration space granulation
(see bus_shift = 24). In order to access configuration space and
probe PEM as ACPI based PCI host controller we need to add MCFG quirk
infrastructure. This involves:
1. Export PEM pci_thunder_pem_ops structure so it is visible to MCFG quirk
   code.
2. New quirk entries for each PEM segment. Each contains platform IDs,
   mentioned pci_thunder_pem_ops and CFG resources.

Quirk is considered for ThunderX silicon pass2.x only which is identified
via MCFG revision 1.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 drivers/acpi/pci_mcfg.c            | 27 +++++++++++++++++++++++++++
 drivers/pci/host/pci-thunder-pem.c |  2 +-
 include/linux/pci-ecam.h           |  4 ++++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index 2b8acc7..1f73d7b 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -51,6 +51,33 @@ struct mcfg_fixup {
 
 static struct mcfg_fixup mcfg_quirks[] = {
 /*	{ OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */
+#ifdef CONFIG_PCI_HOST_THUNDER_PEM
+	/* SoC pass2.x */
+	{ "CAVIUM", "THUNDERX", 1, 4, MCFG_BUS_ANY, &pci_thunder_pem_ops,
+	  DEFINE_RES_MEM(0x88001f000000UL, 0x39 * SZ_16M) },
+	{ "CAVIUM", "THUNDERX", 1, 5, MCFG_BUS_ANY, &pci_thunder_pem_ops,
+	  DEFINE_RES_MEM(0x884057000000UL, 0x39 * SZ_16M) },
+	{ "CAVIUM", "THUNDERX", 1, 6, MCFG_BUS_ANY, &pci_thunder_pem_ops,
+	  DEFINE_RES_MEM(0x88808f000000UL, 0x39 * SZ_16M) },
+	{ "CAVIUM", "THUNDERX", 1, 7, MCFG_BUS_ANY, &pci_thunder_pem_ops,
+	  DEFINE_RES_MEM(0x89001f000000UL, 0x39 * SZ_16M) },
+	{ "CAVIUM", "THUNDERX", 1, 8, MCFG_BUS_ANY, &pci_thunder_pem_ops,
+	  DEFINE_RES_MEM(0x894057000000UL, 0x39 * SZ_16M) },
+	{ "CAVIUM", "THUNDERX", 1, 9, MCFG_BUS_ANY, &pci_thunder_pem_ops,
+	  DEFINE_RES_MEM(0x89808f000000UL, 0x39 * SZ_16M) },
+	{ "CAVIUM", "THUNDERX", 1, 14, MCFG_BUS_ANY, &pci_thunder_pem_ops,
+	  DEFINE_RES_MEM(0x98001f000000UL, 0x39 * SZ_16M) },
+	{ "CAVIUM", "THUNDERX", 1, 15, MCFG_BUS_ANY, &pci_thunder_pem_ops,
+	  DEFINE_RES_MEM(0x984057000000UL, 0x39 * SZ_16M) },
+	{ "CAVIUM", "THUNDERX", 1, 16, MCFG_BUS_ANY, &pci_thunder_pem_ops,
+	  DEFINE_RES_MEM(0x98808f000000UL, 0x39 * SZ_16M) },
+	{ "CAVIUM", "THUNDERX", 1, 17, MCFG_BUS_ANY, &pci_thunder_pem_ops,
+	  DEFINE_RES_MEM(0x99001f000000UL, 0x39 * SZ_16M) },
+	{ "CAVIUM", "THUNDERX", 1, 18, MCFG_BUS_ANY, &pci_thunder_pem_ops,
+	  DEFINE_RES_MEM(0x994057000000UL, 0x39 * SZ_16M) },
+	{ "CAVIUM", "THUNDERX", 1, 19, MCFG_BUS_ANY, &pci_thunder_pem_ops,
+	  DEFINE_RES_MEM(0x99808f000000UL, 0x39 * SZ_16M) },
+#endif
 };
 
 static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
index b048761..d7c10cc 100644
--- a/drivers/pci/host/pci-thunder-pem.c
+++ b/drivers/pci/host/pci-thunder-pem.c
@@ -367,7 +367,7 @@ static int thunder_pem_init(struct pci_config_window *cfg)
 	return 0;
 }
 
-static struct pci_ecam_ops pci_thunder_pem_ops = {
+struct pci_ecam_ops pci_thunder_pem_ops = {
 	.bus_shift	= 24,
 	.init		= thunder_pem_init,
 	.pci_ops	= {
diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
index 7adad20..65505ea 100644
--- a/include/linux/pci-ecam.h
+++ b/include/linux/pci-ecam.h
@@ -58,6 +58,10 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
 			       int where);
 /* default ECAM ops */
 extern struct pci_ecam_ops pci_generic_ecam_ops;
+/* ECAM ops for known quirks */
+#ifdef CONFIG_PCI_HOST_THUNDER_PEM
+extern struct pci_ecam_ops pci_thunder_pem_ops;
+#endif
 
 #ifdef CONFIG_PCI_HOST_GENERIC
 /* for DT-based PCI controllers that support ECAM */
-- 
1.9.1

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

#1486617 — Re: [PATCH V6 4/5] PCI: thunder: Enable ACPI PCI controller for ThunderX pass2.x silicon version

On Fri, Sep 09, 2016 at 09:24:06PM +0200, Tomasz Nowicki wrote:
> ThunderX PCIe controller to off-chip devices (so-called PEM) is not fully
> compliant with ECAM standard. It uses non-standard configuration space
> accessors (see pci_thunder_pem_ops) and custom configuration space granulation
> (see bus_shift = 24). In order to access configuration space and
> probe PEM as ACPI based PCI host controller we need to add MCFG quirk
> infrastructure. This involves:
> 1. Export PEM pci_thunder_pem_ops structure so it is visible to MCFG quirk
>    code.
> 2. New quirk entries for each PEM segment. Each contains platform IDs,
>    mentioned pci_thunder_pem_ops and CFG resources.
> 
> Quirk is considered for ThunderX silicon pass2.x only which is identified
> via MCFG revision 1.

Is it really the case that silicon pass2.x has MCFG revision 1, and
silicon pass1.x has MCFG revision 2?  That just seems backwards.

> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> ---
>  drivers/acpi/pci_mcfg.c            | 27 +++++++++++++++++++++++++++
>  drivers/pci/host/pci-thunder-pem.c |  2 +-
>  include/linux/pci-ecam.h           |  4 ++++
>  3 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index 2b8acc7..1f73d7b 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -51,6 +51,33 @@ struct mcfg_fixup {
>  
>  static struct mcfg_fixup mcfg_quirks[] = {
>  /*	{ OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */
> +#ifdef CONFIG_PCI_HOST_THUNDER_PEM
> +	/* SoC pass2.x */
> +	{ "CAVIUM", "THUNDERX", 1, 4, MCFG_BUS_ANY, &pci_thunder_pem_ops,
> +	  DEFINE_RES_MEM(0x88001f000000UL, 0x39 * SZ_16M) },
> +	{ "CAVIUM", "THUNDERX", 1, 5, MCFG_BUS_ANY, &pci_thunder_pem_ops,
> +	  DEFINE_RES_MEM(0x884057000000UL, 0x39 * SZ_16M) },
> +	{ "CAVIUM", "THUNDERX", 1, 6, MCFG_BUS_ANY, &pci_thunder_pem_ops,
> +	  DEFINE_RES_MEM(0x88808f000000UL, 0x39 * SZ_16M) },
> +	{ "CAVIUM", "THUNDERX", 1, 7, MCFG_BUS_ANY, &pci_thunder_pem_ops,
> +	  DEFINE_RES_MEM(0x89001f000000UL, 0x39 * SZ_16M) },
> +	{ "CAVIUM", "THUNDERX", 1, 8, MCFG_BUS_ANY, &pci_thunder_pem_ops,
> +	  DEFINE_RES_MEM(0x894057000000UL, 0x39 * SZ_16M) },
> +	{ "CAVIUM", "THUNDERX", 1, 9, MCFG_BUS_ANY, &pci_thunder_pem_ops,
> +	  DEFINE_RES_MEM(0x89808f000000UL, 0x39 * SZ_16M) },
> +	{ "CAVIUM", "THUNDERX", 1, 14, MCFG_BUS_ANY, &pci_thunder_pem_ops,
> +	  DEFINE_RES_MEM(0x98001f000000UL, 0x39 * SZ_16M) },
> +	{ "CAVIUM", "THUNDERX", 1, 15, MCFG_BUS_ANY, &pci_thunder_pem_ops,
> +	  DEFINE_RES_MEM(0x984057000000UL, 0x39 * SZ_16M) },
> +	{ "CAVIUM", "THUNDERX", 1, 16, MCFG_BUS_ANY, &pci_thunder_pem_ops,
> +	  DEFINE_RES_MEM(0x98808f000000UL, 0x39 * SZ_16M) },
> +	{ "CAVIUM", "THUNDERX", 1, 17, MCFG_BUS_ANY, &pci_thunder_pem_ops,
> +	  DEFINE_RES_MEM(0x99001f000000UL, 0x39 * SZ_16M) },
> +	{ "CAVIUM", "THUNDERX", 1, 18, MCFG_BUS_ANY, &pci_thunder_pem_ops,
> +	  DEFINE_RES_MEM(0x994057000000UL, 0x39 * SZ_16M) },
> +	{ "CAVIUM", "THUNDERX", 1, 19, MCFG_BUS_ANY, &pci_thunder_pem_ops,
> +	  DEFINE_RES_MEM(0x99808f000000UL, 0x39 * SZ_16M) },
> +#endif
>  };
>  
>  static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
> diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
> index b048761..d7c10cc 100644
> --- a/drivers/pci/host/pci-thunder-pem.c
> +++ b/drivers/pci/host/pci-thunder-pem.c
> @@ -367,7 +367,7 @@ static int thunder_pem_init(struct pci_config_window *cfg)
>  	return 0;
>  }
>  
> -static struct pci_ecam_ops pci_thunder_pem_ops = {
> +struct pci_ecam_ops pci_thunder_pem_ops = {
>  	.bus_shift	= 24,
>  	.init		= thunder_pem_init,
>  	.pci_ops	= {
> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
> index 7adad20..65505ea 100644
> --- a/include/linux/pci-ecam.h
> +++ b/include/linux/pci-ecam.h
> @@ -58,6 +58,10 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
>  			       int where);
>  /* default ECAM ops */
>  extern struct pci_ecam_ops pci_generic_ecam_ops;
> +/* ECAM ops for known quirks */
> +#ifdef CONFIG_PCI_HOST_THUNDER_PEM
> +extern struct pci_ecam_ops pci_thunder_pem_ops;
> +#endif
>  
>  #ifdef CONFIG_PCI_HOST_GENERIC
>  /* for DT-based PCI controllers that support ECAM */
> -- 
> 1.9.1
> 

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

#1487112 — Re: [PATCH V6 4/5] PCI: thunder: Enable ACPI PCI controller for ThunderX pass2.x silicon version

On 19.09.2016 17:45, Bjorn Helgaas wrote:
> On Fri, Sep 09, 2016 at 09:24:06PM +0200, Tomasz Nowicki wrote:
>> ThunderX PCIe controller to off-chip devices (so-called PEM) is not fully
>> compliant with ECAM standard. It uses non-standard configuration space
>> accessors (see pci_thunder_pem_ops) and custom configuration space granulation
>> (see bus_shift = 24). In order to access configuration space and
>> probe PEM as ACPI based PCI host controller we need to add MCFG quirk
>> infrastructure. This involves:
>> 1. Export PEM pci_thunder_pem_ops structure so it is visible to MCFG quirk
>>    code.
>> 2. New quirk entries for each PEM segment. Each contains platform IDs,
>>    mentioned pci_thunder_pem_ops and CFG resources.
>>
>> Quirk is considered for ThunderX silicon pass2.x only which is identified
>> via MCFG revision 1.
>
> Is it really the case that silicon pass2.x has MCFG revision 1, and
> silicon pass1.x has MCFG revision 2?  That just seems backwards.

It is weird but silicon pass2.x is more common and it had MCFG revision 
1 from the beginning. Unless it is allowed to use MCFG revision 0 ? Then 
we could use MCFG revision 0 for pass1.x

Tomasz

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

#1487336 — Re: [PATCH V6 4/5] PCI: thunder: Enable ACPI PCI controller for ThunderX pass2.x silicon version

On Tue, Sep 20, 2016 at 09:06:23AM +0200, Tomasz Nowicki wrote:
> On 19.09.2016 17:45, Bjorn Helgaas wrote:
> >On Fri, Sep 09, 2016 at 09:24:06PM +0200, Tomasz Nowicki wrote:
> >>ThunderX PCIe controller to off-chip devices (so-called PEM) is not fully
> >>compliant with ECAM standard. It uses non-standard configuration space
> >>accessors (see pci_thunder_pem_ops) and custom configuration space granulation
> >>(see bus_shift = 24). In order to access configuration space and
> >>probe PEM as ACPI based PCI host controller we need to add MCFG quirk
> >>infrastructure. This involves:
> >>1. Export PEM pci_thunder_pem_ops structure so it is visible to MCFG quirk
> >>   code.
> >>2. New quirk entries for each PEM segment. Each contains platform IDs,
> >>   mentioned pci_thunder_pem_ops and CFG resources.
> >>
> >>Quirk is considered for ThunderX silicon pass2.x only which is identified
> >>via MCFG revision 1.
> >
> >Is it really the case that silicon pass2.x has MCFG revision 1, and
> >silicon pass1.x has MCFG revision 2?  That just seems backwards.
> 
> It is weird but silicon pass2.x is more common and it had MCFG
> revision 1 from the beginning. Unless it is allowed to use MCFG
> revision 0 ? Then we could use MCFG revision 0 for pass1.x

There's no reason to avoid revision 0.  The question is really what
firmware is already in the field.  We need to accommodate that.  We don't
want a situation where kernel version X only works with firmware version Y,
but kernel version X+1 only works with firmware version Y+1.

Bjorn

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

#1487891 — Re: [PATCH V6 4/5] PCI: thunder: Enable ACPI PCI controller for ThunderX pass2.x silicon version

On 20.09.2016 15:08, Bjorn Helgaas wrote:
> On Tue, Sep 20, 2016 at 09:06:23AM +0200, Tomasz Nowicki wrote:
>> On 19.09.2016 17:45, Bjorn Helgaas wrote:
>>> On Fri, Sep 09, 2016 at 09:24:06PM +0200, Tomasz Nowicki wrote:
>>>> ThunderX PCIe controller to off-chip devices (so-called PEM) is not fully
>>>> compliant with ECAM standard. It uses non-standard configuration space
>>>> accessors (see pci_thunder_pem_ops) and custom configuration space granulation
>>>> (see bus_shift = 24). In order to access configuration space and
>>>> probe PEM as ACPI based PCI host controller we need to add MCFG quirk
>>>> infrastructure. This involves:
>>>> 1. Export PEM pci_thunder_pem_ops structure so it is visible to MCFG quirk
>>>>   code.
>>>> 2. New quirk entries for each PEM segment. Each contains platform IDs,
>>>>   mentioned pci_thunder_pem_ops and CFG resources.
>>>>
>>>> Quirk is considered for ThunderX silicon pass2.x only which is identified
>>>> via MCFG revision 1.
>>>
>>> Is it really the case that silicon pass2.x has MCFG revision 1, and
>>> silicon pass1.x has MCFG revision 2?  That just seems backwards.
>>
>> It is weird but silicon pass2.x is more common and it had MCFG
>> revision 1 from the beginning. Unless it is allowed to use MCFG
>> revision 0 ? Then we could use MCFG revision 0 for pass1.x
>
> There's no reason to avoid revision 0.  The question is really what
> firmware is already in the field.  We need to accommodate that.  We don't
> want a situation where kernel version X only works with firmware version Y,
> but kernel version X+1 only works with firmware version Y+1.

Yes I agree. We have already deployed the firmware where:
pass2.x has MCFG revision 1
pass1.x has MCFG revision 2
so we need to stick to this.

Thanks,
Tomasz

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

#1480279 — [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case

thunder-pem driver stands for being ACPI based PCI host controller.
However, there is no standard way to describe its PEM-specific register
ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension
to obtain hardcoded addresses from static resource array.
Although it is not pretty, it prevents from creating standard mechanism to
handle similar cases in future.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
index 6abaf80..b048761 100644
--- a/drivers/pci/host/pci-thunder-pem.c
+++ b/drivers/pci/host/pci-thunder-pem.c
@@ -18,6 +18,7 @@
 #include <linux/init.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
+#include <linux/pci-acpi.h>
 #include <linux/pci-ecam.h>
 #include <linux/platform_device.h>
 
@@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
 	return pci_generic_config_write(bus, devfn, where, size, val);
 }
 
+#ifdef CONFIG_ACPI
+static struct resource thunder_pem_reg_res[] = {
+	[4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
+	[5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),
+	[6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
+	[7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
+	[8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
+	[9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
+	[14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
+	[15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
+	[16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
+	[17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
+	[18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
+	[19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),
+};
+
+static struct resource *thunder_pem_acpi_res(struct pci_config_window *cfg)
+{
+	struct acpi_device *adev = to_acpi_device(cfg->parent);
+	struct acpi_pci_root *root = acpi_driver_data(adev);
+
+	if ((root->segment >= 4 && root->segment <= 9) ||
+	    (root->segment >= 14 && root->segment <= 19))
+		return &thunder_pem_reg_res[root->segment];
+
+	return NULL;
+}
+#else
+static struct resource *thunder_pem_acpi_res(struct pci_config_window *cfg)
+{
+	return NULL;
+}
+#endif
+
 static int thunder_pem_init(struct pci_config_window *cfg)
 {
 	struct device *dev = cfg->parent;
@@ -292,24 +327,24 @@ static int thunder_pem_init(struct pci_config_window *cfg)
 	struct thunder_pem_pci *pem_pci;
 	struct platform_device *pdev;
 
-	/* Only OF support for now */
-	if (!dev->of_node)
-		return -EINVAL;
-
 	pem_pci = devm_kzalloc(dev, sizeof(*pem_pci), GFP_KERNEL);
 	if (!pem_pci)
 		return -ENOMEM;
 
-	pdev = to_platform_device(dev);
-
-	/*
-	 * The second register range is the PEM bridge to the PCIe
-	 * bus.  It has a different config access method than those
-	 * devices behind the bridge.
-	 */
-	res_pem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (acpi_disabled) {
+		pdev = to_platform_device(dev);
+
+		/*
+		 * The second register range is the PEM bridge to the PCIe
+		 * bus.  It has a different config access method than those
+		 * devices behind the bridge.
+		 */
+		res_pem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	} else {
+		res_pem = thunder_pem_acpi_res(cfg);
+	}
 	if (!res_pem) {
-		dev_err(dev, "missing \"reg[1]\"property\n");
+		dev_err(dev, "missing configuration region\n");
 		return -EINVAL;
 	}
 
-- 
1.9.1

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

#1486735 — Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case

On Fri, Sep 09, 2016 at 09:24:05PM +0200, Tomasz Nowicki wrote:
> thunder-pem driver stands for being ACPI based PCI host controller.
> However, there is no standard way to describe its PEM-specific register
> ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension
> to obtain hardcoded addresses from static resource array.
> Although it is not pretty, it prevents from creating standard mechanism to
> handle similar cases in future.
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> ---
>  drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------
>  1 file changed, 48 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
> index 6abaf80..b048761 100644
> --- a/drivers/pci/host/pci-thunder-pem.c
> +++ b/drivers/pci/host/pci-thunder-pem.c
> @@ -18,6 +18,7 @@
>  #include <linux/init.h>
>  #include <linux/of_address.h>
>  #include <linux/of_pci.h>
> +#include <linux/pci-acpi.h>
>  #include <linux/pci-ecam.h>
>  #include <linux/platform_device.h>
>  
> @@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
>  	return pci_generic_config_write(bus, devfn, where, size, val);
>  }
>  
> +#ifdef CONFIG_ACPI
> +static struct resource thunder_pem_reg_res[] = {
> +	[4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
> +	[5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),
> +	[6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
> +	[7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
> +	[8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
> +	[9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
> +	[14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
> +	[15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
> +	[16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
> +	[17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
> +	[18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
> +	[19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),

1) The "correct" way to discover the resources consumed by an ACPI
   device is to use the _CRS method.  I know there are some issues
   there for bridges (not the fault of ThunderX!) because there's not
   a good way to distinguish windows from resources consumed directly
   by the bridge.

   But we should either do this correctly, or include a comment about
   why we're doing it wrong, so we don't give the impression that this
   is the right way to do it.

   I seem to recall some discussion about why we're doing it this way,
   but I don't remember the details.  It'd be nice to include a
   summary here.

2) This is a little weird because here we define the resource size as
   16MB, in the OF case we get the resource size from OF, in either
   case we ioremap 64K of it, and then as far as I can tell, we only
   ever access PEM_CFG_WR and PEM_CFG_RD, at offsets 0x28 and 0x30
   into the space.

   If the hardware actually decodes the entire 16MB, we should ioremap
   the whole 16MB.  (Strictly speaking, drivers only need to ioremap
   the parts they're using, but in this case nobody claims the entire
   resource because of deficiencies in the ACPI and OF cores, so the
   driver should ioremap the entire thing to help prevent conflicts
   with other devices.)

   It'd be nice if we didn't have the 64KB magic number.  I think
   using devm_ioremap_resource() would be nice.

> +};
> +
> +static struct resource *thunder_pem_acpi_res(struct pci_config_window *cfg)
> +{
> +	struct acpi_device *adev = to_acpi_device(cfg->parent);
> +	struct acpi_pci_root *root = acpi_driver_data(adev);
> +
> +	if ((root->segment >= 4 && root->segment <= 9) ||
> +	    (root->segment >= 14 && root->segment <= 19))
> +		return &thunder_pem_reg_res[root->segment];
> +
> +	return NULL;
> +}
> +#else
> +static struct resource *thunder_pem_acpi_res(struct pci_config_window *cfg)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  static int thunder_pem_init(struct pci_config_window *cfg)
>  {
>  	struct device *dev = cfg->parent;
> @@ -292,24 +327,24 @@ static int thunder_pem_init(struct pci_config_window *cfg)
>  	struct thunder_pem_pci *pem_pci;
>  	struct platform_device *pdev;
>  
> -	/* Only OF support for now */
> -	if (!dev->of_node)
> -		return -EINVAL;
> -
>  	pem_pci = devm_kzalloc(dev, sizeof(*pem_pci), GFP_KERNEL);
>  	if (!pem_pci)
>  		return -ENOMEM;
>  
> -	pdev = to_platform_device(dev);
> -
> -	/*
> -	 * The second register range is the PEM bridge to the PCIe
> -	 * bus.  It has a different config access method than those
> -	 * devices behind the bridge.
> -	 */
> -	res_pem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (acpi_disabled) {
> +		pdev = to_platform_device(dev);
> +
> +		/*
> +		 * The second register range is the PEM bridge to the PCIe
> +		 * bus.  It has a different config access method than those
> +		 * devices behind the bridge.
> +		 */
> +		res_pem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	} else {
> +		res_pem = thunder_pem_acpi_res(cfg);
> +	}
>  	if (!res_pem) {
> -		dev_err(dev, "missing \"reg[1]\"property\n");
> +		dev_err(dev, "missing configuration region\n");
>  		return -EINVAL;
>  	}
>  
> -- 
> 1.9.1
> 

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

#1487117 — Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case

On 19.09.2016 20:09, Bjorn Helgaas wrote:
> On Fri, Sep 09, 2016 at 09:24:05PM +0200, Tomasz Nowicki wrote:
>> thunder-pem driver stands for being ACPI based PCI host controller.
>> However, there is no standard way to describe its PEM-specific register
>> ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension
>> to obtain hardcoded addresses from static resource array.
>> Although it is not pretty, it prevents from creating standard mechanism to
>> handle similar cases in future.
>>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> ---
>>  drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 48 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
>> index 6abaf80..b048761 100644
>> --- a/drivers/pci/host/pci-thunder-pem.c
>> +++ b/drivers/pci/host/pci-thunder-pem.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/init.h>
>>  #include <linux/of_address.h>
>>  #include <linux/of_pci.h>
>> +#include <linux/pci-acpi.h>
>>  #include <linux/pci-ecam.h>
>>  #include <linux/platform_device.h>
>>
>> @@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
>>  	return pci_generic_config_write(bus, devfn, where, size, val);
>>  }
>>
>> +#ifdef CONFIG_ACPI
>> +static struct resource thunder_pem_reg_res[] = {
>> +	[4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
>> +	[5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),
>> +	[6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
>> +	[7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
>> +	[8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
>> +	[9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
>> +	[14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
>> +	[15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
>> +	[16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
>> +	[17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
>> +	[18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
>> +	[19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),
>
> 1) The "correct" way to discover the resources consumed by an ACPI
>    device is to use the _CRS method.  I know there are some issues
>    there for bridges (not the fault of ThunderX!) because there's not
>    a good way to distinguish windows from resources consumed directly
>    by the bridge.
>
>    But we should either do this correctly, or include a comment about
>    why we're doing it wrong, so we don't give the impression that this
>    is the right way to do it.
>
>    I seem to recall some discussion about why we're doing it this way,
>    but I don't remember the details.  It'd be nice to include a
>    summary here.

OK I will. The reason why we cannot use _CRS for this case is that 
CONSUMER flag was not use consistently for the bridge so far.

>
> 2) This is a little weird because here we define the resource size as
>    16MB, in the OF case we get the resource size from OF, in either
>    case we ioremap 64K of it, and then as far as I can tell, we only
>    ever access PEM_CFG_WR and PEM_CFG_RD, at offsets 0x28 and 0x30
>    into the space.
>
>    If the hardware actually decodes the entire 16MB, we should ioremap
>    the whole 16MB.  (Strictly speaking, drivers only need to ioremap
>    the parts they're using, but in this case nobody claims the entire
>    resource because of deficiencies in the ACPI and OF cores, so the
>    driver should ioremap the entire thing to help prevent conflicts
>    with other devices.)
>
>    It'd be nice if we didn't have the 64KB magic number.  I think
>    using devm_ioremap_resource() would be nice.

I agree.

David, is there anything which prevents us from using 
devm_ioremap_resource() here with SZ_16M size?

Tomasz

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

#1487344 — Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case

[+cc Rafael (maybe already cc'd; I didn't recognize rafael@kernel.org, Duc]

On Tue, Sep 20, 2016 at 09:23:21AM +0200, Tomasz Nowicki wrote:
> On 19.09.2016 20:09, Bjorn Helgaas wrote:
> >On Fri, Sep 09, 2016 at 09:24:05PM +0200, Tomasz Nowicki wrote:
> >>thunder-pem driver stands for being ACPI based PCI host controller.
> >>However, there is no standard way to describe its PEM-specific register
> >>ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension
> >>to obtain hardcoded addresses from static resource array.
> >>Although it is not pretty, it prevents from creating standard mechanism to
> >>handle similar cases in future.
> >>
> >>Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> >>---
> >> drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------
> >> 1 file changed, 48 insertions(+), 13 deletions(-)
> >>
> >>diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
> >>index 6abaf80..b048761 100644
> >>--- a/drivers/pci/host/pci-thunder-pem.c
> >>+++ b/drivers/pci/host/pci-thunder-pem.c
> >>@@ -18,6 +18,7 @@
> >> #include <linux/init.h>
> >> #include <linux/of_address.h>
> >> #include <linux/of_pci.h>
> >>+#include <linux/pci-acpi.h>
> >> #include <linux/pci-ecam.h>
> >> #include <linux/platform_device.h>
> >>
> >>@@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
> >> 	return pci_generic_config_write(bus, devfn, where, size, val);
> >> }
> >>
> >>+#ifdef CONFIG_ACPI
> >>+static struct resource thunder_pem_reg_res[] = {
> >>+	[4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
> >>+	[5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),
> >>+	[6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
> >>+	[7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
> >>+	[8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
> >>+	[9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
> >>+	[14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
> >>+	[15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
> >>+	[16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
> >>+	[17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
> >>+	[18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
> >>+	[19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),
> >
> >1) The "correct" way to discover the resources consumed by an ACPI
> >   device is to use the _CRS method.  I know there are some issues
> >   there for bridges (not the fault of ThunderX!) because there's not
> >   a good way to distinguish windows from resources consumed directly
> >   by the bridge.
> >
> >   But we should either do this correctly, or include a comment about
> >   why we're doing it wrong, so we don't give the impression that this
> >   is the right way to do it.
> >
> >   I seem to recall some discussion about why we're doing it this way,
> >   but I don't remember the details.  It'd be nice to include a
> >   summary here.
> 
> OK I will. The reason why we cannot use _CRS for this case is that
> CONSUMER flag was not use consistently for the bridge so far.

Yes, I'm aware of that problem, but hard-coding resources into drivers
is just a disaster.  The PCI and ACPI cores need generic ways to learn
what resources are consumed by devices.  For PCI devices, that's done
with BARs.  For ACPI devices, it's done with _CRS.  Without generic
resource discovery, we can't manage resources reliably at the system
level [1].

You have a PNP0A03/PNP0A08 device for the PCI host bridge.  Because of
the BIOS bugs in CONSUMER flag usage, we assume everything in its _CRS
is a window and not consumed by the bridge itself.  What if you added
a companion ACPI device with a _CRS that contained the bridge
resources?  Then you'd have some driver ugliness to find that device,
but at least the ACPI core could tell what resources were in use.

Maybe Rafael has a better idea?

Bjorn

[1] I know the ACPI core currently doesn't actually *do* anything with
_CRS.  But I think it *should*, and someday it might, so I want to
preserve the principle of using _CRS to document all the resources.

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

#1487351 — Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case

On 20 September 2016 at 14:33, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Rafael (maybe already cc'd; I didn't recognize rafael@kernel.org, Duc]
>
> On Tue, Sep 20, 2016 at 09:23:21AM +0200, Tomasz Nowicki wrote:
>> On 19.09.2016 20:09, Bjorn Helgaas wrote:
>> >On Fri, Sep 09, 2016 at 09:24:05PM +0200, Tomasz Nowicki wrote:
>> >>thunder-pem driver stands for being ACPI based PCI host controller.
>> >>However, there is no standard way to describe its PEM-specific register
>> >>ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension
>> >>to obtain hardcoded addresses from static resource array.
>> >>Although it is not pretty, it prevents from creating standard mechanism to
>> >>handle similar cases in future.
>> >>
>> >>Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> >>---
>> >> drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------
>> >> 1 file changed, 48 insertions(+), 13 deletions(-)
>> >>
>> >>diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
>> >>index 6abaf80..b048761 100644
>> >>--- a/drivers/pci/host/pci-thunder-pem.c
>> >>+++ b/drivers/pci/host/pci-thunder-pem.c
>> >>@@ -18,6 +18,7 @@
>> >> #include <linux/init.h>
>> >> #include <linux/of_address.h>
>> >> #include <linux/of_pci.h>
>> >>+#include <linux/pci-acpi.h>
>> >> #include <linux/pci-ecam.h>
>> >> #include <linux/platform_device.h>
>> >>
>> >>@@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
>> >>    return pci_generic_config_write(bus, devfn, where, size, val);
>> >> }
>> >>
>> >>+#ifdef CONFIG_ACPI
>> >>+static struct resource thunder_pem_reg_res[] = {
>> >>+   [4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
>> >>+   [5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),
>> >>+   [6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
>> >>+   [7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
>> >>+   [8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
>> >>+   [9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
>> >>+   [14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
>> >>+   [15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
>> >>+   [16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
>> >>+   [17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
>> >>+   [18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
>> >>+   [19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),
>> >
>> >1) The "correct" way to discover the resources consumed by an ACPI
>> >   device is to use the _CRS method.  I know there are some issues
>> >   there for bridges (not the fault of ThunderX!) because there's not
>> >   a good way to distinguish windows from resources consumed directly
>> >   by the bridge.
>> >
>> >   But we should either do this correctly, or include a comment about
>> >   why we're doing it wrong, so we don't give the impression that this
>> >   is the right way to do it.
>> >
>> >   I seem to recall some discussion about why we're doing it this way,
>> >   but I don't remember the details.  It'd be nice to include a
>> >   summary here.
>>
>> OK I will. The reason why we cannot use _CRS for this case is that
>> CONSUMER flag was not use consistently for the bridge so far.
>
> Yes, I'm aware of that problem, but hard-coding resources into drivers
> is just a disaster.  The PCI and ACPI cores need generic ways to learn
> what resources are consumed by devices.  For PCI devices, that's done
> with BARs.  For ACPI devices, it's done with _CRS.  Without generic
> resource discovery, we can't manage resources reliably at the system
> level [1].
>
> You have a PNP0A03/PNP0A08 device for the PCI host bridge.  Because of
> the BIOS bugs in CONSUMER flag usage, we assume everything in its _CRS
> is a window and not consumed by the bridge itself.  What if you added
> a companion ACPI device with a _CRS that contained the bridge
> resources?  Then you'd have some driver ugliness to find that device,
> but at least the ACPI core could tell what resources were in use.
>
> Maybe Rafael has a better idea?
>

In the discussions leading up to this, we tried very hard to make this
arm64/acpi quirks mechanism just as flexible as we need it to be to
cover the current crop of incompatible hardware, but not more so.
Going forward, we intend to require all arm64/acpi hardware to be spec
compliant, and so any parametrization beyond what is required for the
currently known broken hardware is only going to make it easier for
others to ship with tweaked ACPI descriptions so that an existing
quirk is triggered for hardware that it was not intended for. It also
implies that we have to deal with the ACPI descriptions as they were
shipped with the current hardware.

That does not mean, of course, that we should use bare constants
rather than symbolic ones, but anything beyond that exceeds the
desired scope of quirks handling.

-- 
Ard.

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

#1487378 — Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case

Hi Ard,

On Tue, Sep 20, 2016 at 02:40:13PM +0100, Ard Biesheuvel wrote:
> On 20 September 2016 at 14:33, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > [+cc Rafael (maybe already cc'd; I didn't recognize rafael@kernel.org, Duc]
> >
> > On Tue, Sep 20, 2016 at 09:23:21AM +0200, Tomasz Nowicki wrote:
> >> On 19.09.2016 20:09, Bjorn Helgaas wrote:
> >> >On Fri, Sep 09, 2016 at 09:24:05PM +0200, Tomasz Nowicki wrote:
> >> >>thunder-pem driver stands for being ACPI based PCI host controller.
> >> >>However, there is no standard way to describe its PEM-specific register
> >> >>ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension
> >> >>to obtain hardcoded addresses from static resource array.
> >> >>Although it is not pretty, it prevents from creating standard mechanism to
> >> >>handle similar cases in future.
> >> >>
> >> >>Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> >> >>---
> >> >> drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------
> >> >> 1 file changed, 48 insertions(+), 13 deletions(-)
> >> >>
> >> >>diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
> >> >>index 6abaf80..b048761 100644
> >> >>--- a/drivers/pci/host/pci-thunder-pem.c
> >> >>+++ b/drivers/pci/host/pci-thunder-pem.c
> >> >>@@ -18,6 +18,7 @@
> >> >> #include <linux/init.h>
> >> >> #include <linux/of_address.h>
> >> >> #include <linux/of_pci.h>
> >> >>+#include <linux/pci-acpi.h>
> >> >> #include <linux/pci-ecam.h>
> >> >> #include <linux/platform_device.h>
> >> >>
> >> >>@@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
> >> >>    return pci_generic_config_write(bus, devfn, where, size, val);
> >> >> }
> >> >>
> >> >>+#ifdef CONFIG_ACPI
> >> >>+static struct resource thunder_pem_reg_res[] = {
> >> >>+   [4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
> >> >>+   [5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),
> >> >>+   [6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
> >> >>+   [7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
> >> >>+   [8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
> >> >>+   [9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
> >> >>+   [14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
> >> >>+   [15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
> >> >>+   [16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
> >> >>+   [17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
> >> >>+   [18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
> >> >>+   [19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),
> >> >
> >> >1) The "correct" way to discover the resources consumed by an ACPI
> >> >   device is to use the _CRS method.  I know there are some issues
> >> >   there for bridges (not the fault of ThunderX!) because there's not
> >> >   a good way to distinguish windows from resources consumed directly
> >> >   by the bridge.
> >> >
> >> >   But we should either do this correctly, or include a comment about
> >> >   why we're doing it wrong, so we don't give the impression that this
> >> >   is the right way to do it.
> >> >
> >> >   I seem to recall some discussion about why we're doing it this way,
> >> >   but I don't remember the details.  It'd be nice to include a
> >> >   summary here.
> >>
> >> OK I will. The reason why we cannot use _CRS for this case is that
> >> CONSUMER flag was not use consistently for the bridge so far.
> >
> > Yes, I'm aware of that problem, but hard-coding resources into drivers
> > is just a disaster.  The PCI and ACPI cores need generic ways to learn
> > what resources are consumed by devices.  For PCI devices, that's done
> > with BARs.  For ACPI devices, it's done with _CRS.  Without generic
> > resource discovery, we can't manage resources reliably at the system
> > level [1].
> >
> > You have a PNP0A03/PNP0A08 device for the PCI host bridge.  Because of
> > the BIOS bugs in CONSUMER flag usage, we assume everything in its _CRS
> > is a window and not consumed by the bridge itself.  What if you added
> > a companion ACPI device with a _CRS that contained the bridge
> > resources?  Then you'd have some driver ugliness to find that device,
> > but at least the ACPI core could tell what resources were in use.
> >
> > Maybe Rafael has a better idea?
> 
> In the discussions leading up to this, we tried very hard to make this
> arm64/acpi quirks mechanism just as flexible as we need it to be to
> cover the current crop of incompatible hardware, but not more so.
> Going forward, we intend to require all arm64/acpi hardware to be spec
> compliant, and so any parametrization beyond what is required for the
> currently known broken hardware is only going to make it easier for
> others to ship with tweaked ACPI descriptions so that an existing
> quirk is triggered for hardware that it was not intended for. It also
> implies that we have to deal with the ACPI descriptions as they were
> shipped with the current hardware.
> 
> That does not mean, of course, that we should use bare constants
> rather than symbolic ones, but anything beyond that exceeds the
> desired scope of quirks handling.

Symbolic vs bare constants is the least of my worries.  I'm pretty
happy with the current quirk implementation.  It's pretty simple and
straightforward.

Apparently you shipped broken firmware that doesn't accurately
describe system resource usage.  Presumably that firmware could be
updated, but maybe it's worthwhile to work around it in the kernel,
depending on where it got shipped.

I'd like to step back and come up with some understanding of how
non-broken firmware *should* deal with this issue.  Then, if we *do*
work around this particular broken firmware in the kernel, it would be
nice to do it in a way that fits in with that understanding.

For example, if a companion ACPI device is the preferred solution, an
ACPI quirk could fabricate a device with the required resources.  That
would address the problem closer to the source and make it more likely
that the rest of the system will work correctly: /proc/iomem could
make sense, things that look at _CRS generically would work (e.g,
/sys/, an admittedly hypothetical "lsacpi", etc.)

Hard-coding stuff in drivers is a point solution that doesn't provide
any guidance for future platforms and makes it likely that the hack
will get copied into even more drivers.

Bjorn

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

#1487414 — Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case

On 20 September 2016 at 15:05, Bjorn Helgaas <helgaas@kernel.org> wrote:
> Hi Ard,
>
> On Tue, Sep 20, 2016 at 02:40:13PM +0100, Ard Biesheuvel wrote:
>> On 20 September 2016 at 14:33, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > [+cc Rafael (maybe already cc'd; I didn't recognize rafael@kernel.org, Duc]
>> >
>> > On Tue, Sep 20, 2016 at 09:23:21AM +0200, Tomasz Nowicki wrote:
>> >> On 19.09.2016 20:09, Bjorn Helgaas wrote:
>> >> >On Fri, Sep 09, 2016 at 09:24:05PM +0200, Tomasz Nowicki wrote:
>> >> >>thunder-pem driver stands for being ACPI based PCI host controller.
>> >> >>However, there is no standard way to describe its PEM-specific register
>> >> >>ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension
>> >> >>to obtain hardcoded addresses from static resource array.
>> >> >>Although it is not pretty, it prevents from creating standard mechanism to
>> >> >>handle similar cases in future.
>> >> >>
>> >> >>Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> >> >>---
>> >> >> drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------
>> >> >> 1 file changed, 48 insertions(+), 13 deletions(-)
>> >> >>
>> >> >>diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
>> >> >>index 6abaf80..b048761 100644
>> >> >>--- a/drivers/pci/host/pci-thunder-pem.c
>> >> >>+++ b/drivers/pci/host/pci-thunder-pem.c
>> >> >>@@ -18,6 +18,7 @@
>> >> >> #include <linux/init.h>
>> >> >> #include <linux/of_address.h>
>> >> >> #include <linux/of_pci.h>
>> >> >>+#include <linux/pci-acpi.h>
>> >> >> #include <linux/pci-ecam.h>
>> >> >> #include <linux/platform_device.h>
>> >> >>
>> >> >>@@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
>> >> >>    return pci_generic_config_write(bus, devfn, where, size, val);
>> >> >> }
>> >> >>
>> >> >>+#ifdef CONFIG_ACPI
>> >> >>+static struct resource thunder_pem_reg_res[] = {
>> >> >>+   [4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
>> >> >>+   [5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),
>> >> >>+   [6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
>> >> >>+   [7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
>> >> >>+   [8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
>> >> >>+   [9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
>> >> >>+   [14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
>> >> >>+   [15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
>> >> >>+   [16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
>> >> >>+   [17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
>> >> >>+   [18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
>> >> >>+   [19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),
>> >> >
>> >> >1) The "correct" way to discover the resources consumed by an ACPI
>> >> >   device is to use the _CRS method.  I know there are some issues
>> >> >   there for bridges (not the fault of ThunderX!) because there's not
>> >> >   a good way to distinguish windows from resources consumed directly
>> >> >   by the bridge.
>> >> >
>> >> >   But we should either do this correctly, or include a comment about
>> >> >   why we're doing it wrong, so we don't give the impression that this
>> >> >   is the right way to do it.
>> >> >
>> >> >   I seem to recall some discussion about why we're doing it this way,
>> >> >   but I don't remember the details.  It'd be nice to include a
>> >> >   summary here.
>> >>
>> >> OK I will. The reason why we cannot use _CRS for this case is that
>> >> CONSUMER flag was not use consistently for the bridge so far.
>> >
>> > Yes, I'm aware of that problem, but hard-coding resources into drivers
>> > is just a disaster.  The PCI and ACPI cores need generic ways to learn
>> > what resources are consumed by devices.  For PCI devices, that's done
>> > with BARs.  For ACPI devices, it's done with _CRS.  Without generic
>> > resource discovery, we can't manage resources reliably at the system
>> > level [1].
>> >
>> > You have a PNP0A03/PNP0A08 device for the PCI host bridge.  Because of
>> > the BIOS bugs in CONSUMER flag usage, we assume everything in its _CRS
>> > is a window and not consumed by the bridge itself.  What if you added
>> > a companion ACPI device with a _CRS that contained the bridge
>> > resources?  Then you'd have some driver ugliness to find that device,
>> > but at least the ACPI core could tell what resources were in use.
>> >
>> > Maybe Rafael has a better idea?
>>
>> In the discussions leading up to this, we tried very hard to make this
>> arm64/acpi quirks mechanism just as flexible as we need it to be to
>> cover the current crop of incompatible hardware, but not more so.
>> Going forward, we intend to require all arm64/acpi hardware to be spec
>> compliant, and so any parametrization beyond what is required for the
>> currently known broken hardware is only going to make it easier for
>> others to ship with tweaked ACPI descriptions so that an existing
>> quirk is triggered for hardware that it was not intended for. It also
>> implies that we have to deal with the ACPI descriptions as they were
>> shipped with the current hardware.
>>
>> That does not mean, of course, that we should use bare constants
>> rather than symbolic ones, but anything beyond that exceeds the
>> desired scope of quirks handling.
>
> Symbolic vs bare constants is the least of my worries.  I'm pretty
> happy with the current quirk implementation.  It's pretty simple and
> straightforward.
>

OK, good to know that we are on the right track here.

> Apparently you shipped broken firmware that doesn't accurately
> describe system resource usage.  Presumably that firmware could be
> updated, but maybe it's worthwhile to work around it in the kernel,
> depending on where it got shipped.
>

None of these platforms can be fixed entirely in software, and given
that we will not be adding quirks for new broken hardware, we should
ask ourselves whether having two versions of a quirk, i.e., one for
broken hardware + currently shipping firmware, and one for the same
broken hardware with fixed firmware is really an improvement over what
has been proposed here.

> I'd like to step back and come up with some understanding of how
> non-broken firmware *should* deal with this issue.  Then, if we *do*
> work around this particular broken firmware in the kernel, it would be
> nice to do it in a way that fits in with that understanding.
>
> For example, if a companion ACPI device is the preferred solution, an
> ACPI quirk could fabricate a device with the required resources.  That
> would address the problem closer to the source and make it more likely
> that the rest of the system will work correctly: /proc/iomem could
> make sense, things that look at _CRS generically would work (e.g,
> /sys/, an admittedly hypothetical "lsacpi", etc.)
>
> Hard-coding stuff in drivers is a point solution that doesn't provide
> any guidance for future platforms and makes it likely that the hack
> will get copied into even more drivers.
>

OK, I see. But the guidance for future platforms should be 'do not
rely on quirks', and what I am arguing here is that the more we polish
up this code and make it clean and reusable, the more likely it is
that will end up getting abused by new broken hardware that we set out
to reject entirely in the first place.

So of course, if the quirk involves claiming resources, let's make
sure that this occurs in the cleanest and most compliant way possible.
But any factoring/reuse concerns other than for the current crop of
broken hardware should be avoided imo.

-- 
Ard.

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

#1487566 — Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case

On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:
> On 20 September 2016 at 15:05, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > Hi Ard,
> >
> > On Tue, Sep 20, 2016 at 02:40:13PM +0100, Ard Biesheuvel wrote:
> >> On 20 September 2016 at 14:33, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> > [+cc Rafael (maybe already cc'd; I didn't recognize rafael@kernel.org, Duc]
> >> >
> >> > On Tue, Sep 20, 2016 at 09:23:21AM +0200, Tomasz Nowicki wrote:
> >> >> On 19.09.2016 20:09, Bjorn Helgaas wrote:
> >> >> >On Fri, Sep 09, 2016 at 09:24:05PM +0200, Tomasz Nowicki wrote:
> >> >> >>thunder-pem driver stands for being ACPI based PCI host controller.
> >> >> >>However, there is no standard way to describe its PEM-specific register
> >> >> >>ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension
> >> >> >>to obtain hardcoded addresses from static resource array.
> >> >> >>Although it is not pretty, it prevents from creating standard mechanism to
> >> >> >>handle similar cases in future.
> >> >> >>
> >> >> >>Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> >> >> >>---
> >> >> >> drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------
> >> >> >> 1 file changed, 48 insertions(+), 13 deletions(-)
> >> >> >>
> >> >> >>diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
> >> >> >>index 6abaf80..b048761 100644
> >> >> >>--- a/drivers/pci/host/pci-thunder-pem.c
> >> >> >>+++ b/drivers/pci/host/pci-thunder-pem.c
> >> >> >>@@ -18,6 +18,7 @@
> >> >> >> #include <linux/init.h>
> >> >> >> #include <linux/of_address.h>
> >> >> >> #include <linux/of_pci.h>
> >> >> >>+#include <linux/pci-acpi.h>
> >> >> >> #include <linux/pci-ecam.h>
> >> >> >> #include <linux/platform_device.h>
> >> >> >>
> >> >> >>@@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
> >> >> >>    return pci_generic_config_write(bus, devfn, where, size, val);
> >> >> >> }
> >> >> >>
> >> >> >>+#ifdef CONFIG_ACPI
> >> >> >>+static struct resource thunder_pem_reg_res[] = {
> >> >> >>+   [4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
> >> >> >>+   [5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),
> >> >> >>+   [6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
> >> >> >>+   [7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
> >> >> >>+   [8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
> >> >> >>+   [9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
> >> >> >>+   [14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
> >> >> >>+   [15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
> >> >> >>+   [16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
> >> >> >>+   [17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
> >> >> >>+   [18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
> >> >> >>+   [19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),
> >> >> >
> >> >> >1) The "correct" way to discover the resources consumed by an ACPI
> >> >> >   device is to use the _CRS method.  I know there are some issues
> >> >> >   there for bridges (not the fault of ThunderX!) because there's not
> >> >> >   a good way to distinguish windows from resources consumed directly
> >> >> >   by the bridge.
> >> >> >
> >> >> >   But we should either do this correctly, or include a comment about
> >> >> >   why we're doing it wrong, so we don't give the impression that this
> >> >> >   is the right way to do it.
> >> >> >
> >> >> >   I seem to recall some discussion about why we're doing it this way,
> >> >> >   but I don't remember the details.  It'd be nice to include a
> >> >> >   summary here.
> >> >>
> >> >> OK I will. The reason why we cannot use _CRS for this case is that
> >> >> CONSUMER flag was not use consistently for the bridge so far.
> >> >
> >> > Yes, I'm aware of that problem, but hard-coding resources into drivers
> >> > is just a disaster.  The PCI and ACPI cores need generic ways to learn
> >> > what resources are consumed by devices.  For PCI devices, that's done
> >> > with BARs.  For ACPI devices, it's done with _CRS.  Without generic
> >> > resource discovery, we can't manage resources reliably at the system
> >> > level [1].
> >> >
> >> > You have a PNP0A03/PNP0A08 device for the PCI host bridge.  Because of
> >> > the BIOS bugs in CONSUMER flag usage, we assume everything in its _CRS
> >> > is a window and not consumed by the bridge itself.  What if you added
> >> > a companion ACPI device with a _CRS that contained the bridge
> >> > resources?  Then you'd have some driver ugliness to find that device,
> >> > but at least the ACPI core could tell what resources were in use.
> >> >
> >> > Maybe Rafael has a better idea?
> >>
> >> In the discussions leading up to this, we tried very hard to make this
> >> arm64/acpi quirks mechanism just as flexible as we need it to be to
> >> cover the current crop of incompatible hardware, but not more so.
> >> Going forward, we intend to require all arm64/acpi hardware to be spec
> >> compliant, and so any parametrization beyond what is required for the
> >> currently known broken hardware is only going to make it easier for
> >> others to ship with tweaked ACPI descriptions so that an existing
> >> quirk is triggered for hardware that it was not intended for. It also
> >> implies that we have to deal with the ACPI descriptions as they were
> >> shipped with the current hardware.
> >>
> >> That does not mean, of course, that we should use bare constants
> >> rather than symbolic ones, but anything beyond that exceeds the
> >> desired scope of quirks handling.
> >
> > Symbolic vs bare constants is the least of my worries.  I'm pretty
> > happy with the current quirk implementation.  It's pretty simple and
> > straightforward.
> >
> 
> OK, good to know that we are on the right track here.
> 
> > Apparently you shipped broken firmware that doesn't accurately
> > describe system resource usage.  Presumably that firmware could be
> > updated, but maybe it's worthwhile to work around it in the kernel,
> > depending on where it got shipped.
> >
> 
> None of these platforms can be fixed entirely in software, and given
> that we will not be adding quirks for new broken hardware, we should
> ask ourselves whether having two versions of a quirk, i.e., one for
> broken hardware + currently shipping firmware, and one for the same
> broken hardware with fixed firmware is really an improvement over what
> has been proposed here.

We're talking about two completely different types of quirks:

  1) MCFG quirks to use memory-mapped config space that doesn't quite
     conform to the ECAM model in the PCIe spec, and

  2) Some yet-to-be-determined method to describe address space
     consumed by a bridge.

The first two patches of this series are a nice implementation for 1).
The third patch (ThunderX-specific) is one possibility for 2), but I
don't like it because there's no way for generic software like the
ACPI core to discover these resources.

> > I'd like to step back and come up with some understanding of how
> > non-broken firmware *should* deal with this issue.  Then, if we *do*
> > work around this particular broken firmware in the kernel, it would be
> > nice to do it in a way that fits in with that understanding.
> >
> > For example, if a companion ACPI device is the preferred solution, an
> > ACPI quirk could fabricate a device with the required resources.  That
> > would address the problem closer to the source and make it more likely
> > that the rest of the system will work correctly: /proc/iomem could
> > make sense, things that look at _CRS generically would work (e.g,
> > /sys/, an admittedly hypothetical "lsacpi", etc.)
> >
> > Hard-coding stuff in drivers is a point solution that doesn't provide
> > any guidance for future platforms and makes it likely that the hack
> > will get copied into even more drivers.
> >
> 
> OK, I see. But the guidance for future platforms should be 'do not
> rely on quirks', and what I am arguing here is that the more we polish
> up this code and make it clean and reusable, the more likely it is
> that will end up getting abused by new broken hardware that we set out
> to reject entirely in the first place.
> 
> So of course, if the quirk involves claiming resources, let's make
> sure that this occurs in the cleanest and most compliant way possible.
> But any factoring/reuse concerns other than for the current crop of
> broken hardware should be avoided imo.

If future hardware is completely ECAM-compliant and we don't need any
more MCFG quirks, that would be great.

But we'll still need to describe that memory-mapped config space
somewhere.  If that's done with PNP0C02 or similar devices (as is done
on my x86 laptop), we'd be all set.

If we need to work around firmware in the field that doesn't do that,
one possibility is a PNP quirk along the lines of
quirk_amd_mmconfig_area().

Bjorn

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

#1488144 — Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case

On Tue, Sep 20, 2016 at 02:17:44PM -0500, Bjorn Helgaas wrote:
> On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:

[...]

> > None of these platforms can be fixed entirely in software, and given
> > that we will not be adding quirks for new broken hardware, we should
> > ask ourselves whether having two versions of a quirk, i.e., one for
> > broken hardware + currently shipping firmware, and one for the same
> > broken hardware with fixed firmware is really an improvement over what
> > has been proposed here.
> 
> We're talking about two completely different types of quirks:
> 
>   1) MCFG quirks to use memory-mapped config space that doesn't quite
>      conform to the ECAM model in the PCIe spec, and
> 
>   2) Some yet-to-be-determined method to describe address space
>      consumed by a bridge.
> 
> The first two patches of this series are a nice implementation for 1).
> The third patch (ThunderX-specific) is one possibility for 2), but I
> don't like it because there's no way for generic software like the
> ACPI core to discover these resources.

Ok, so basically this means that to implement (2) we need to assign
some sort of _HID to these quirky PCI bridges (so that we know what
device they represent and we can retrieve their _CRS). I take from
this discussion that the goal is to make sure that all non-config
resources have to be declared through _CRS device objects, which is
fine but that requires a FW update (unless we can fabricate ACPI
devices and corresponding _CRS in the kernel whenever we match a
given MCFG table signature).

We discussed this already and I think we should make a decision:

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/414722.html

> > > I'd like to step back and come up with some understanding of how
> > > non-broken firmware *should* deal with this issue.  Then, if we *do*
> > > work around this particular broken firmware in the kernel, it would be
> > > nice to do it in a way that fits in with that understanding.
> > >
> > > For example, if a companion ACPI device is the preferred solution, an
> > > ACPI quirk could fabricate a device with the required resources.  That
> > > would address the problem closer to the source and make it more likely
> > > that the rest of the system will work correctly: /proc/iomem could
> > > make sense, things that look at _CRS generically would work (e.g,
> > > /sys/, an admittedly hypothetical "lsacpi", etc.)
> > >
> > > Hard-coding stuff in drivers is a point solution that doesn't provide
> > > any guidance for future platforms and makes it likely that the hack
> > > will get copied into even more drivers.
> > >
> > 
> > OK, I see. But the guidance for future platforms should be 'do not
> > rely on quirks', and what I am arguing here is that the more we polish
> > up this code and make it clean and reusable, the more likely it is
> > that will end up getting abused by new broken hardware that we set out
> > to reject entirely in the first place.
> > 
> > So of course, if the quirk involves claiming resources, let's make
> > sure that this occurs in the cleanest and most compliant way possible.
> > But any factoring/reuse concerns other than for the current crop of
> > broken hardware should be avoided imo.
> 
> If future hardware is completely ECAM-compliant and we don't need any
> more MCFG quirks, that would be great.

Yes.

> But we'll still need to describe that memory-mapped config space
> somewhere.  If that's done with PNP0C02 or similar devices (as is done
> on my x86 laptop), we'd be all set.

I am not sure I understand what you mean here. Are you referring
to MCFG regions reported as PNP0c02 resources through its _CRS ?

IIUC PNP0C02 is a reservation mechanism, but it does not help us
associate its _CRS to a specific PCI host bridge instance, right ?

> If we need to work around firmware in the field that doesn't do that,
> one possibility is a PNP quirk along the lines of
> quirk_amd_mmconfig_area().

You mean matching PNP0C01/PNP0c02 and create a resource (that has to
hardcoded in a static array in the kernel anyway, there is no way to
retrieve it otherwise) in the corresponding PNP quirk handler ?

And it is not a given we can match against PNP0c01/PNP0c02.

So it looks like the only solution is allocating an _HID for each
host bridge that is not ECAM compliant to add resources to its _CRS
(unless the MCFG quirk does not need any additional data/resource,
eg "use different set of PCI accessorsi 32-bit vs byte-access").

For FW that is immutable I really do not see what we can do apart
from hardcoding the non-config resources (consumed by a bridge),
somehow.

Thanks,
Lorenzo

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

#1488311 — Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case

On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:
> On Tue, Sep 20, 2016 at 02:17:44PM -0500, Bjorn Helgaas wrote:
> > On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:
> 
> [...]
> 
> > > None of these platforms can be fixed entirely in software, and given
> > > that we will not be adding quirks for new broken hardware, we should
> > > ask ourselves whether having two versions of a quirk, i.e., one for
> > > broken hardware + currently shipping firmware, and one for the same
> > > broken hardware with fixed firmware is really an improvement over what
> > > has been proposed here.
> > 
> > We're talking about two completely different types of quirks:
> > 
> >   1) MCFG quirks to use memory-mapped config space that doesn't quite
> >      conform to the ECAM model in the PCIe spec, and
> > 
> >   2) Some yet-to-be-determined method to describe address space
> >      consumed by a bridge.
> > 
> > The first two patches of this series are a nice implementation for 1).
> > The third patch (ThunderX-specific) is one possibility for 2), but I
> > don't like it because there's no way for generic software like the
> > ACPI core to discover these resources.
> 
> Ok, so basically this means that to implement (2) we need to assign
> some sort of _HID to these quirky PCI bridges (so that we know what
> device they represent and we can retrieve their _CRS). I take from
> this discussion that the goal is to make sure that all non-config
> resources have to be declared through _CRS device objects, which is
> fine but that requires a FW update (unless we can fabricate ACPI
> devices and corresponding _CRS in the kernel whenever we match a
> given MCFG table signature).

All resources consumed by ACPI devices should be declared through
_CRS.  If you want to fabricate ACPI devices or _CRS via kernel
quirks, that's fine with me.  This could be triggered via MCFG
signature, DMI info, host bridge _HID, etc.

> We discussed this already and I think we should make a decision:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/414722.html
> 
> > > > I'd like to step back and come up with some understanding of how
> > > > non-broken firmware *should* deal with this issue.  Then, if we *do*
> > > > work around this particular broken firmware in the kernel, it would be
> > > > nice to do it in a way that fits in with that understanding.
> > > >
> > > > For example, if a companion ACPI device is the preferred solution, an
> > > > ACPI quirk could fabricate a device with the required resources.  That
> > > > would address the problem closer to the source and make it more likely
> > > > that the rest of the system will work correctly: /proc/iomem could
> > > > make sense, things that look at _CRS generically would work (e.g,
> > > > /sys/, an admittedly hypothetical "lsacpi", etc.)
> > > >
> > > > Hard-coding stuff in drivers is a point solution that doesn't provide
> > > > any guidance for future platforms and makes it likely that the hack
> > > > will get copied into even more drivers.
> > > >
> > > 
> > > OK, I see. But the guidance for future platforms should be 'do not
> > > rely on quirks', and what I am arguing here is that the more we polish
> > > up this code and make it clean and reusable, the more likely it is
> > > that will end up getting abused by new broken hardware that we set out
> > > to reject entirely in the first place.
> > > 
> > > So of course, if the quirk involves claiming resources, let's make
> > > sure that this occurs in the cleanest and most compliant way possible.
> > > But any factoring/reuse concerns other than for the current crop of
> > > broken hardware should be avoided imo.
> > 
> > If future hardware is completely ECAM-compliant and we don't need any
> > more MCFG quirks, that would be great.
> 
> Yes.
> 
> > But we'll still need to describe that memory-mapped config space
> > somewhere.  If that's done with PNP0C02 or similar devices (as is done
> > on my x86 laptop), we'd be all set.
> 
> I am not sure I understand what you mean here. Are you referring
> to MCFG regions reported as PNP0c02 resources through its _CRS ?

Yes.  PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges
reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02
device.

> IIUC PNP0C02 is a reservation mechanism, but it does not help us
> associate its _CRS to a specific PCI host bridge instance, right ?

Gab proposed a hierarchy that *would* associate a PNP0C02 device with
a PCI bridge:

  Device (PCI1) {
    Name (_HID, "HISI0080") // PCI Express Root Bridge
    Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
    Method (_CRS, 0, Serialized) { // Root complex resources (windows) }
    Device (RES0) {
      Name (_HID, "HISI0081") // HiSi PCIe RC config base address
      Name (_CID, "PNP0C02")  // Motherboard reserved resource
      Name (_CRS, ResourceTemplate () { ... }
    }
  }

That's a possibility.  The PCI Firmware Spec suggests putting RES0 at
the root (under \_SB), but I don't know why.

Putting it at the root means we couldn't generically associate it with
a bridge, although I could imagine something like this:

  Device (RES1) {
    Name (_HID, "HISI0081") // HiSi PCIe RC config base address
    Name (_CID, "PNP0C02")  // Motherboard reserved resource
    Name (_CRS, ResourceTemplate () { ... }
    Method (BRDG) { "PCI1" }  // hand-wavy ASL
  }
  Device (PCI1) {
    Name (_HID, "HISI0080") // PCI Express Root Bridge
    Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
    Method (_CRS, 0, Serialized) { // Root complex resources (windows) }
  }

Where you could search PNP0C02 devices for a cookie that matched the
host bridge.

> > If we need to work around firmware in the field that doesn't do that,
> > one possibility is a PNP quirk along the lines of
> > quirk_amd_mmconfig_area().
> 
> You mean matching PNP0C01/PNP0c02 and create a resource (that has to
> hardcoded in a static array in the kernel anyway, there is no way to
> retrieve it otherwise) in the corresponding PNP quirk handler ?

Right.  On some hardware we can read the resource out of a
device-specific register, as we do in quirk_intel_mch().  But if
that's not possible, it would have to be hard-coded.

> And it is not a given we can match against PNP0c01/PNP0c02.
> 
> So it looks like the only solution is allocating an _HID for each
> host bridge that is not ECAM compliant to add resources to its _CRS
> (unless the MCFG quirk does not need any additional data/resource,
> eg "use different set of PCI accessorsi 32-bit vs byte-access").

It doesn't matter whether it's ECAM-compliant or not.  Any
memory-mapped config space should be reported via some device's _CRS.

The existing x86 practice is to use PNP0C02 devices for this purpose,
and I think we should just follow that practice.

> For FW that is immutable I really do not see what we can do apart
> from hardcoding the non-config resources (consumed by a bridge),
> somehow.

Right.  Well, I assume you mean we should hard-code "non-window
resources consumed directly by a bridge".  If firmware in the field is
broken, we should work around it, and that may mean hard-coding some
resources.

My point is that the hard-coding should not be buried in a driver
where it's invisible to the rest of the kernel.  If we hard-code it in
a quirk that adds _CRS entries, then the kernel will work just like it
would if the firmware had been correct in the first place.  The
resource will appear in /sys/devices/pnp*/*/resources and /proc/iomem,
and if we ever used _SRS to assign or move ACPI devices, we would know
to avoid the bridge resource.

Bjorn

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

#1488334 — Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case

On Wed, Sep 21, 2016 at 11:04 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:
>> On Tue, Sep 20, 2016 at 02:17:44PM -0500, Bjorn Helgaas wrote:
>> > On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:
>>
>> [...]
>>
>> > > None of these platforms can be fixed entirely in software, and given
>> > > that we will not be adding quirks for new broken hardware, we should
>> > > ask ourselves whether having two versions of a quirk, i.e., one for
>> > > broken hardware + currently shipping firmware, and one for the same
>> > > broken hardware with fixed firmware is really an improvement over what
>> > > has been proposed here.
>> >
>> > We're talking about two completely different types of quirks:
>> >
>> >   1) MCFG quirks to use memory-mapped config space that doesn't quite
>> >      conform to the ECAM model in the PCIe spec, and
>> >
>> >   2) Some yet-to-be-determined method to describe address space
>> >      consumed by a bridge.
>> >
>> > The first two patches of this series are a nice implementation for 1).
>> > The third patch (ThunderX-specific) is one possibility for 2), but I
>> > don't like it because there's no way for generic software like the
>> > ACPI core to discover these resources.
>>
>> Ok, so basically this means that to implement (2) we need to assign
>> some sort of _HID to these quirky PCI bridges (so that we know what
>> device they represent and we can retrieve their _CRS). I take from
>> this discussion that the goal is to make sure that all non-config
>> resources have to be declared through _CRS device objects, which is
>> fine but that requires a FW update (unless we can fabricate ACPI
>> devices and corresponding _CRS in the kernel whenever we match a
>> given MCFG table signature).
>
> All resources consumed by ACPI devices should be declared through
> _CRS.  If you want to fabricate ACPI devices or _CRS via kernel
> quirks, that's fine with me.  This could be triggered via MCFG
> signature, DMI info, host bridge _HID, etc.
>
>> We discussed this already and I think we should make a decision:
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/414722.html
>>
>> > > > I'd like to step back and come up with some understanding of how
>> > > > non-broken firmware *should* deal with this issue.  Then, if we *do*
>> > > > work around this particular broken firmware in the kernel, it would be
>> > > > nice to do it in a way that fits in with that understanding.
>> > > >
>> > > > For example, if a companion ACPI device is the preferred solution, an
>> > > > ACPI quirk could fabricate a device with the required resources.  That
>> > > > would address the problem closer to the source and make it more likely
>> > > > that the rest of the system will work correctly: /proc/iomem could
>> > > > make sense, things that look at _CRS generically would work (e.g,
>> > > > /sys/, an admittedly hypothetical "lsacpi", etc.)
>> > > >
>> > > > Hard-coding stuff in drivers is a point solution that doesn't provide
>> > > > any guidance for future platforms and makes it likely that the hack
>> > > > will get copied into even more drivers.
>> > > >
>> > >
>> > > OK, I see. But the guidance for future platforms should be 'do not
>> > > rely on quirks', and what I am arguing here is that the more we polish
>> > > up this code and make it clean and reusable, the more likely it is
>> > > that will end up getting abused by new broken hardware that we set out
>> > > to reject entirely in the first place.
>> > >
>> > > So of course, if the quirk involves claiming resources, let's make
>> > > sure that this occurs in the cleanest and most compliant way possible.
>> > > But any factoring/reuse concerns other than for the current crop of
>> > > broken hardware should be avoided imo.
>> >
>> > If future hardware is completely ECAM-compliant and we don't need any
>> > more MCFG quirks, that would be great.
>>
>> Yes.
>>
>> > But we'll still need to describe that memory-mapped config space
>> > somewhere.  If that's done with PNP0C02 or similar devices (as is done
>> > on my x86 laptop), we'd be all set.
>>
>> I am not sure I understand what you mean here. Are you referring
>> to MCFG regions reported as PNP0c02 resources through its _CRS ?
>
> Yes.  PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges
> reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02
> device.
>
>> IIUC PNP0C02 is a reservation mechanism, but it does not help us
>> associate its _CRS to a specific PCI host bridge instance, right ?
>
> Gab proposed a hierarchy that *would* associate a PNP0C02 device with
> a PCI bridge:
>
>   Device (PCI1) {
>     Name (_HID, "HISI0080") // PCI Express Root Bridge
>     Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
>     Method (_CRS, 0, Serialized) { // Root complex resources (windows) }
>     Device (RES0) {
>       Name (_HID, "HISI0081") // HiSi PCIe RC config base address
>       Name (_CID, "PNP0C02")  // Motherboard reserved resource
>       Name (_CRS, ResourceTemplate () { ... }
>     }
>   }
>
> That's a possibility.  The PCI Firmware Spec suggests putting RES0 at
> the root (under \_SB), but I don't know why.
>
> Putting it at the root means we couldn't generically associate it with
> a bridge, although I could imagine something like this:
>
>   Device (RES1) {
>     Name (_HID, "HISI0081") // HiSi PCIe RC config base address
>     Name (_CID, "PNP0C02")  // Motherboard reserved resource
>     Name (_CRS, ResourceTemplate () { ... }
>     Method (BRDG) { "PCI1" }  // hand-wavy ASL
>   }
>   Device (PCI1) {
>     Name (_HID, "HISI0080") // PCI Express Root Bridge
>     Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
>     Method (_CRS, 0, Serialized) { // Root complex resources (windows) }
>   }
>
> Where you could search PNP0C02 devices for a cookie that matched the
> host bridge.
>
>> > If we need to work around firmware in the field that doesn't do that,
>> > one possibility is a PNP quirk along the lines of
>> > quirk_amd_mmconfig_area().
>>
>> You mean matching PNP0C01/PNP0c02 and create a resource (that has to
>> hardcoded in a static array in the kernel anyway, there is no way to
>> retrieve it otherwise) in the corresponding PNP quirk handler ?
>
> Right.  On some hardware we can read the resource out of a
> device-specific register, as we do in quirk_intel_mch().  But if
> that's not possible, it would have to be hard-coded.
>
>> And it is not a given we can match against PNP0c01/PNP0c02.
>>
>> So it looks like the only solution is allocating an _HID for each
>> host bridge that is not ECAM compliant to add resources to its _CRS
>> (unless the MCFG quirk does not need any additional data/resource,
>> eg "use different set of PCI accessorsi 32-bit vs byte-access").
>
> It doesn't matter whether it's ECAM-compliant or not.  Any
> memory-mapped config space should be reported via some device's _CRS.
>
> The existing x86 practice is to use PNP0C02 devices for this purpose,
> and I think we should just follow that practice.
>
>> For FW that is immutable I really do not see what we can do apart
>> from hardcoding the non-config resources (consumed by a bridge),
>> somehow.
>
> Right.  Well, I assume you mean we should hard-code "non-window
> resources consumed directly by a bridge".  If firmware in the field is
> broken, we should work around it, and that may mean hard-coding some
> resources.
>
> My point is that the hard-coding should not be buried in a driver
> where it's invisible to the rest of the kernel.  If we hard-code it in
> a quirk that adds _CRS entries, then the kernel will work just like it
> would if the firmware had been correct in the first place.  The
> resource will appear in /sys/devices/pnp*/*/resources and /proc/iomem,
> and if we ever used _SRS to assign or move ACPI devices, we would know
> to avoid the bridge resource.

Hi Bjorn,

Are you suggesting to add code similar to functions in
linux/drivers/pnp/quirks.c to declare/attach the additional resource
that the host need to have when the resource is not in MCFG table?

>
> Bjorn
Regards,
Duc Dang.

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

#1488341 — Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case

On Wed, Sep 21, 2016 at 11:58:22AM -0700, Duc Dang wrote:
> On Wed, Sep 21, 2016 at 11:04 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:

> > The existing x86 practice is to use PNP0C02 devices for this purpose,
> > and I think we should just follow that practice.
> >
> > ...
> >
> > My point is that the hard-coding should not be buried in a driver
> > where it's invisible to the rest of the kernel.  If we hard-code it in
> > a quirk that adds _CRS entries, then the kernel will work just like it
> > would if the firmware had been correct in the first place.  The
> > resource will appear in /sys/devices/pnp*/*/resources and /proc/iomem,
> > and if we ever used _SRS to assign or move ACPI devices, we would know
> > to avoid the bridge resource.
> 
> Are you suggesting to add code similar to functions in
> linux/drivers/pnp/quirks.c to declare/attach the additional resource
> that the host need to have when the resource is not in MCFG table?

Yes, but what I'm suggesting is actually a little stronger.  This has
nothing to do with whether a resource is in the MCFG table or not.

I'm suggesting ACPI firmware should always describe the resource.  If the
firmware is defective and doesn't describe it, we should add a quirk in
pnp/quirks.c to add a resource for it.

Bjorn

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

#1489942 — Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case

Hi Bjorn,

On 21.09.2016 21:18, Bjorn Helgaas wrote:
> On Wed, Sep 21, 2016 at 11:58:22AM -0700, Duc Dang wrote:
>> On Wed, Sep 21, 2016 at 11:04 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:
>
>>> The existing x86 practice is to use PNP0C02 devices for this purpose,
>>> and I think we should just follow that practice.
>>>
>>> ...
>>>
>>> My point is that the hard-coding should not be buried in a driver
>>> where it's invisible to the rest of the kernel.  If we hard-code it in
>>> a quirk that adds _CRS entries, then the kernel will work just like it
>>> would if the firmware had been correct in the first place.  The
>>> resource will appear in /sys/devices/pnp*/*/resources and /proc/iomem,
>>> and if we ever used _SRS to assign or move ACPI devices, we would know
>>> to avoid the bridge resource.
>>
>> Are you suggesting to add code similar to functions in
>> linux/drivers/pnp/quirks.c to declare/attach the additional resource
>> that the host need to have when the resource is not in MCFG table?
>
> Yes, but what I'm suggesting is actually a little stronger.  This has
> nothing to do with whether a resource is in the MCFG table or not.
>
> I'm suggesting ACPI firmware should always describe the resource.  If the
> firmware is defective and doesn't describe it, we should add a quirk in
> pnp/quirks.c to add a resource for it.
>

Thanks for pointers Bjorn.

ThunderX is the case where we cannot change firmware, also it has no 
PNP0c02 device in tables. So in order to use pnp/quirks.c we would have 
to fabricate PNP0c02 in kernel and then add quirk entry. I am looking 
for the best place to put such emulation code but it seems not trivial.

Thanks,
Tomasz

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

#1488699 — Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case

On Wed, Sep 21, 2016 at 01:04:57PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:
> > On Tue, Sep 20, 2016 at 02:17:44PM -0500, Bjorn Helgaas wrote:
> > > On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:
> > 
> > [...]
> > 
> > > > None of these platforms can be fixed entirely in software, and given
> > > > that we will not be adding quirks for new broken hardware, we should
> > > > ask ourselves whether having two versions of a quirk, i.e., one for
> > > > broken hardware + currently shipping firmware, and one for the same
> > > > broken hardware with fixed firmware is really an improvement over what
> > > > has been proposed here.
> > > 
> > > We're talking about two completely different types of quirks:
> > > 
> > >   1) MCFG quirks to use memory-mapped config space that doesn't quite
> > >      conform to the ECAM model in the PCIe spec, and
> > > 
> > >   2) Some yet-to-be-determined method to describe address space
> > >      consumed by a bridge.
> > > 
> > > The first two patches of this series are a nice implementation for 1).
> > > The third patch (ThunderX-specific) is one possibility for 2), but I
> > > don't like it because there's no way for generic software like the
> > > ACPI core to discover these resources.
> > 
> > Ok, so basically this means that to implement (2) we need to assign
> > some sort of _HID to these quirky PCI bridges (so that we know what
> > device they represent and we can retrieve their _CRS). I take from
> > this discussion that the goal is to make sure that all non-config
> > resources have to be declared through _CRS device objects, which is
> > fine but that requires a FW update (unless we can fabricate ACPI
> > devices and corresponding _CRS in the kernel whenever we match a
> > given MCFG table signature).
> 
> All resources consumed by ACPI devices should be declared through
> _CRS.  If you want to fabricate ACPI devices or _CRS via kernel
> quirks, that's fine with me.  This could be triggered via MCFG
> signature, DMI info, host bridge _HID, etc.

I think the PNP quirk approach + PNP0c02 resource put forward by Gab
is enough.

> > We discussed this already and I think we should make a decision:
> > 
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/414722.html
> > 
> > > > > I'd like to step back and come up with some understanding of how
> > > > > non-broken firmware *should* deal with this issue.  Then, if we *do*
> > > > > work around this particular broken firmware in the kernel, it would be
> > > > > nice to do it in a way that fits in with that understanding.
> > > > >
> > > > > For example, if a companion ACPI device is the preferred solution, an
> > > > > ACPI quirk could fabricate a device with the required resources.  That
> > > > > would address the problem closer to the source and make it more likely
> > > > > that the rest of the system will work correctly: /proc/iomem could
> > > > > make sense, things that look at _CRS generically would work (e.g,
> > > > > /sys/, an admittedly hypothetical "lsacpi", etc.)
> > > > >
> > > > > Hard-coding stuff in drivers is a point solution that doesn't provide
> > > > > any guidance for future platforms and makes it likely that the hack
> > > > > will get copied into even more drivers.
> > > > >
> > > > 
> > > > OK, I see. But the guidance for future platforms should be 'do not
> > > > rely on quirks', and what I am arguing here is that the more we polish
> > > > up this code and make it clean and reusable, the more likely it is
> > > > that will end up getting abused by new broken hardware that we set out
> > > > to reject entirely in the first place.
> > > > 
> > > > So of course, if the quirk involves claiming resources, let's make
> > > > sure that this occurs in the cleanest and most compliant way possible.
> > > > But any factoring/reuse concerns other than for the current crop of
> > > > broken hardware should be avoided imo.
> > > 
> > > If future hardware is completely ECAM-compliant and we don't need any
> > > more MCFG quirks, that would be great.
> > 
> > Yes.
> > 
> > > But we'll still need to describe that memory-mapped config space
> > > somewhere.  If that's done with PNP0C02 or similar devices (as is done
> > > on my x86 laptop), we'd be all set.
> > 
> > I am not sure I understand what you mean here. Are you referring
> > to MCFG regions reported as PNP0c02 resources through its _CRS ?
> 
> Yes.  PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges
> reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02
> device.

Ok, that's agreed. It goes without saying that since you are quoting
the PCI spec, if FW fails to report MCFG regions in a PNP0c02 device
_CRS I will consider that a FW bug.

> > IIUC PNP0C02 is a reservation mechanism, but it does not help us
> > associate its _CRS to a specific PCI host bridge instance, right ?
> 
> Gab proposed a hierarchy that *would* associate a PNP0C02 device with
> a PCI bridge:
> 
>   Device (PCI1) {
>     Name (_HID, "HISI0080") // PCI Express Root Bridge
>     Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
>     Method (_CRS, 0, Serialized) { // Root complex resources (windows) }
>     Device (RES0) {
>       Name (_HID, "HISI0081") // HiSi PCIe RC config base address
>       Name (_CID, "PNP0C02")  // Motherboard reserved resource
>       Name (_CRS, ResourceTemplate () { ... }
>     }
>   }
> 
> That's a possibility.  The PCI Firmware Spec suggests putting RES0 at
> the root (under \_SB), but I don't know why.
> 
> Putting it at the root means we couldn't generically associate it with
> a bridge, although I could imagine something like this:
> 
>   Device (RES1) {
>     Name (_HID, "HISI0081") // HiSi PCIe RC config base address
>     Name (_CID, "PNP0C02")  // Motherboard reserved resource
>     Name (_CRS, ResourceTemplate () { ... }
>     Method (BRDG) { "PCI1" }  // hand-wavy ASL
>   }
>   Device (PCI1) {
>     Name (_HID, "HISI0080") // PCI Express Root Bridge
>     Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
>     Method (_CRS, 0, Serialized) { // Root complex resources (windows) }
>   }
> 
> Where you could search PNP0C02 devices for a cookie that matched the
> host bridge.o

Ok, I am fine with both and I think we are converging, but the way
to solve this problem has to be uniform for all ARM partners (and
not only ARM). Two points here:

1) Adding a device/subdevice allows people to add a _CRS reporting the
   non-window bridge resources. Fine. It also allows people to chuck in
   there all sorts of _DSD properties to describe their PCI host bridge
   as it is done with DT properties (those _DSD can contain eg clocks
   etc.), this may be tempting (so that they can reuse the same DT
   driver and do not have to update their firmware) but I want to be
   clear here: that must not happen. So, a subdevice with a _CRS to
   report resources, yes, but it will stop there.
2) It is unclear to me how to formalize the above. People should not
   write FW by reading the PCI mailing list, so these guidelines have to
   be written, somehow. I do not want to standardize quirks, I want
   to prevent random ACPI table content, which is different.
   Should I report this to the ACPI spec working group ? If we do
   not do that everyone will go solve this problem as they deem fit.

[...]

> > For FW that is immutable I really do not see what we can do apart
> > from hardcoding the non-config resources (consumed by a bridge),
> > somehow.
> 
> Right.  Well, I assume you mean we should hard-code "non-window
> resources consumed directly by a bridge".  If firmware in the field is
> broken, we should work around it, and that may mean hard-coding some
> resources.
> 
> My point is that the hard-coding should not be buried in a driver
> where it's invisible to the rest of the kernel.  If we hard-code it in
> a quirk that adds _CRS entries, then the kernel will work just like it
> would if the firmware had been correct in the first place.  The
> resource will appear in /sys/devices/pnp*/*/resources and /proc/iomem,
> and if we ever used _SRS to assign or move ACPI devices, we would know
> to avoid the bridge resource.

We are in complete agreement here.

Thanks,
Lorenzo

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

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web