[PATCH v1 0/4] PCI: microchip: apportion address translation between rootport and FPGA

[PATCH v1 0/4] PCI: microchip: apportion address translation between rootport and FPGA

[daire.mcnamara]

From: Daire McNamara <daire.mcnamara@microchip.com>

Hi all,

This series includes an attempt to deal with the PCIe address translation
behaviour of Microchip PolarFire SoC.  On this chip, FPGA logic exists
between the PCIe rootport and the CPUs (Core Complex). So, outbound and
inbound translation to PCIe devices is achieved by a combination of individual 
customer's FPGA fabric designs and by the PCIe rootport itself and thus
the outbound and inbound address translation specified in range
properties and dma-range properties of the PCIe rootport appears insufficient
as they do not capture how much outbound and inbound address translation
has occured in the FPGA design between the addresses used by the CPU and
the addresses used by the PCIe devices. So, we require some mechanism
to inform the root port of what address translation it actually needs
to perform in order to achieve the goals specified in the range and
dma-range properties.

This series proposes two new Microchip properties, each in the form of
ranges, which capture the amount of outbound and inbound translation
done by the FPGA fabric, if any.  

If the new properties are absent, the range and dma-range properties 
are intended to be parsed by the root port driver as usual, and the 
entire specified address translation is carried out by the root port
using its address translation tables.

if one of the new properties are present, the translations
carried out by the rootport, as specified in the range or dma-range properties,
are adjusted by the amount of address translation carried out by
the FPGA design, as described in the details of the new properties.

The new properties are structured as ranges to enable FPGA designers to have
different address translation ranges; for example, an FPGA designer may
choose to partition 32-bit address translation and 38-bit translation through
different apertures for their particular design or may choose to target
non-cached and/or cached DDR with different dma-ranges.

This series contains a proposed new binding for the properties, and an
implementation of the new properties for the Microchip PolarFire SoC
PCIe rootport driver.

Thanks,
Daire

Conor Dooley (1):
  dt-bindings: PCI: microchip: add fabric address translation properties

Daire McNamara (3):
  riscv: dts: microchip: add fabric address translation properties
  PCI: microchip: add fabric address translation properties
  of: PCI: tidy up logging of ranges containing configuration space type

 .../bindings/pci/microchip,pcie-host.yaml     | 107 ++++++++++++++++++
 .../dts/microchip/mpfs-icicle-kit-fabric.dtsi |   6 +-
 drivers/pci/controller/pcie-microchip-host.c  |  59 ++++++++--
 drivers/pci/of.c                              |   2 +
 4 files changed, 166 insertions(+), 8 deletions(-)


base-commit: 6496a28df951641c0d50052ee195c7765591ff92
prerequisite-patch-id: 39bd182e929a064e38ca191a1726dd6d5a620f2d
prerequisite-patch-id: 9401b90950832090dabfe5f74f525ed4fa1c1410
prerequisite-patch-id: 606a8ca57d3dc19b04490b6e75d267a7c0d76163
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 1/4] dt-bindings: PCI: microchip: add fabric address translation properties

[daire.mcnamara]

From: Conor Dooley <conor.dooley@microchip.com>

On PolarFire SoC both in- & out-bound address translations occur in two
stages. The specific translations are tightly coupled to the FPGA
designs and supplement the {dma-,}ranges properties. The first stage of
the translation is done by the FPGA fabric & the second by the root
port.
Add two properties so that the translation tables in the root port's
bridge layer can be configured to account for the translation done by
the FPGA fabric.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
---
 .../bindings/pci/microchip,pcie-host.yaml     | 107 ++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
index 23d95c65acff..29bb1fe99a2e 100644
--- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
+++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
@@ -71,6 +71,113 @@ properties:
     minItems: 1
     maxItems: 6
 
+  microchip,outbound-fabric-translation-ranges:
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    minItems: 1
+    maxItems: 32
+    description: |
+      The CPU-to-PCIe (outbound) address translation takes place in two stages.
+      Depending on the FPGA bitstream, the outbound address translation tables
+      in the PCIe root port's bridge layer will need to be configured to account
+      for only its part of the overall outbound address translation.
+
+      The first stage of outbound address translation occurs between the CPU address
+      and an intermediate "FPGA address". The second stage of outbound address
+      translation occurs between this FPGA address and the PCIe address. Use this
+      property, in conjunction with the ranges property, to divide the overall
+      address translation into these two stages so that the PCIe address
+      translation tables can be correctly configured.
+
+      If this property is present, one entry is required per range. This is so
+      FPGA designers can choose to route different address ranges through different
+      Fabric Interface Controllers and other logic as they see fit.
+
+      If this property is not present, the entire address translation
+      in any ranges property is attempted by the root port driver via its outbound
+      address translation tables.
+
+      Each element in this property has three components. The first is a
+      PCIe address, the second is an FPGA address, and the third is a size.
+      These properties may be 32 or 64 bit values.
+
+      In operation, the driver will expect a one-to-one correspondance between
+      range properties and this property.  For each pair of range and
+      outbound-fabric-translation-range properties, the root port driver will
+      subtract the FPGA address in this property from the CPU address in the
+      corresponding range property and use the remainder to program its
+      outbound address translation tables.
+
+      For each range, take its PCIe address and size - these are the PCIe
+      address & size for the element. The FPGA address is derived from a given
+      FPGA fabric design and is the address delivered by that FPGA fabric
+      design to the Core Complex. For a trivial configuration, it is likely to be the
+      lower 32 bits of the PCIe address in the range property and the upper
+      bits of the base address of the Fabric Interface Controller the design uses.
+      Otherwise, it is tightly coupled with the data path configured in the
+      FPGA fabric between the root port and the Core Complex.
+
+      For more information on the tables, see Section 1.3.3,
+      "PCIe/AXI4 Address Translation" of the PolarFire SoC PCIe User Guide:
+      https://www.microsemi.com/document-portal/doc_download/1245812-polarfire-fpga-and-polarfire-soc-fpga-pci-express-user-guide
+
+    items:
+      minItems: 3
+      maxItems: 6
+
+  microchip,inbound-fabric-translation-ranges:
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    minItems: 1
+    maxItems: 32
+    description: |
+      The PCIe-to-CPU (inbound) address translation takes place in two stages.
+      Depending on the FPGA bitstream, the inbound address translation tables
+      in the PCIe root port's bridge layer will need to be configured to account
+      for only its part of the overall inbound address translation.
+
+      The first stage of address translation occurs between the PCIe address and
+      an intermediate FPGA address. The second stage of address translation
+      occurs between the FPGA address and the CPU address. Use this property
+      in conjunction with the dma-ranges property to divide the address
+      translation into these two stages.
+
+      If this property is present, one entry is required per dma-range. This is so
+      FPGA designers can choose to route different address ranges through different
+      Fabric Interface Controllers and other logic as they see fit.
+
+      If this property is not present, the entire address translation
+      in any dma-ranges property is attempted by the root port driver via its
+      inbound address translation tables.
+
+      Each element in this property has three components. The first is a
+      PCIe address, the second is an FPGA address, and the third is a size.
+      These properties may be 32 or 64 bit values.
+
+      In operation, the driver will expect a one-to-one correspondance between
+      dma-range properties and this property.  For each pair of dma-range and
+      inbound-fabric-translation-range properties, the root port driver will
+      subtract the FPGA address in this property from the CPU address in the
+      corresponding dma-range property and use the remainder to program its
+      inbound address translation tables.
+
+      From each dma-range, take its PCIe address and size - these are the PCIe
+      address & size for the element. The FPGA address is derived from a given
+      FPGA fabric design and is the address delivered by that FPGA fabric
+      design to the Core Complex. For a trivial configuration, this property
+      is unlikely to be required (i.e. no fabric translation on the inbound
+      interface).  Otherwise, it is tightly coupled with the inbound data path
+      configured in the FPGA fabric between the root port and the Core Complex.
+      It is expected that more than one translation range may be added to
+      an FPGA fabric design, e.g. to deliver data to cached or non-cached
+      DDR.
+
+      For more information on the tables, see Section 1.3.3,
+      "PCIe/AXI4 Address Translation" of the PolarFire SoC PCIe User Guide:
+      https://www.microsemi.com/document-portal/doc_download/1245812-polarfire-fpga-and-polarfire-soc-fpga-pci-express-user-guide
+
+    items:
+      minItems: 4
+      maxItems: 7
+
   msi-controller:
     description: Identifies the node as an MSI controller.
 
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 1/4] dt-bindings: PCI: microchip: add fabric address translation properties

[Rob Herring]

On Fri, Sep 2, 2022 at 9:22 AM <daire.mcnamara@microchip.com> wrote:
>
> From: Conor Dooley <conor.dooley@microchip.com>
>
> On PolarFire SoC both in- & out-bound address translations occur in two
> stages. The specific translations are tightly coupled to the FPGA
> designs and supplement the {dma-,}ranges properties. The first stage of
> the translation is done by the FPGA fabric & the second by the root
> port.
> Add two properties so that the translation tables in the root port's
> bridge layer can be configured to account for the translation done by
> the FPGA fabric.

I'm skeptical that ranges/dma-ranges can't handle what you need.
Anything in this area is going to need justification 'ranges doesn't
work because x, y, z...'.

>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> ---
>  .../bindings/pci/microchip,pcie-host.yaml     | 107 ++++++++++++++++++
>  1 file changed, 107 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> index 23d95c65acff..29bb1fe99a2e 100644
> --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> @@ -71,6 +71,113 @@ properties:
>      minItems: 1
>      maxItems: 6
>
> +  microchip,outbound-fabric-translation-ranges:
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    minItems: 1
> +    maxItems: 32
> +    description: |
> +      The CPU-to-PCIe (outbound) address translation takes place in two stages.
> +      Depending on the FPGA bitstream, the outbound address translation tables
> +      in the PCIe root port's bridge layer will need to be configured to account
> +      for only its part of the overall outbound address translation.
> +
> +      The first stage of outbound address translation occurs between the CPU address
> +      and an intermediate "FPGA address". The second stage of outbound address
> +      translation occurs between this FPGA address and the PCIe address. Use this
> +      property, in conjunction with the ranges property, to divide the overall
> +      address translation into these two stages so that the PCIe address
> +      translation tables can be correctly configured.

Sounds like you need 2 levels of ranges/dma-ranges.

/ {
    fpga-bus {
        ranges = ...
        dma-ranges = ...
        pcie@... {
            ranges = ...
            dma-ranges = ...
        };
    };
};

> +
> +      If this property is present, one entry is required per range. This is so
> +      FPGA designers can choose to route different address ranges through different
> +      Fabric Interface Controllers and other logic as they see fit.
> +
> +      If this property is not present, the entire address translation
> +      in any ranges property is attempted by the root port driver via its outbound
> +      address translation tables.
> +
> +      Each element in this property has three components. The first is a
> +      PCIe address, the second is an FPGA address, and the third is a size.
> +      These properties may be 32 or 64 bit values.
> +
> +      In operation, the driver will expect a one-to-one correspondance between
> +      range properties and this property.  For each pair of range and
> +      outbound-fabric-translation-range properties, the root port driver will
> +      subtract the FPGA address in this property from the CPU address in the
> +      corresponding range property and use the remainder to program its
> +      outbound address translation tables.
> +
> +      For each range, take its PCIe address and size - these are the PCIe
> +      address & size for the element. The FPGA address is derived from a given
> +      FPGA fabric design and is the address delivered by that FPGA fabric
> +      design to the Core Complex. For a trivial configuration, it is likely to be the
> +      lower 32 bits of the PCIe address in the range property and the upper
> +      bits of the base address of the Fabric Interface Controller the design uses.
> +      Otherwise, it is tightly coupled with the data path configured in the
> +      FPGA fabric between the root port and the Core Complex.
> +
> +      For more information on the tables, see Section 1.3.3,
> +      "PCIe/AXI4 Address Translation" of the PolarFire SoC PCIe User Guide:
> +      https://www.microsemi.com/document-portal/doc_download/1245812-polarfire-fpga-and-polarfire-soc-fpga-pci-express-user-guide
> +
> +    items:
> +      minItems: 3
> +      maxItems: 6
> +
> +  microchip,inbound-fabric-translation-ranges:
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    minItems: 1
> +    maxItems: 32
> +    description: |
> +      The PCIe-to-CPU (inbound) address translation takes place in two stages.
> +      Depending on the FPGA bitstream, the inbound address translation tables
> +      in the PCIe root port's bridge layer will need to be configured to account
> +      for only its part of the overall inbound address translation.
> +
> +      The first stage of address translation occurs between the PCIe address and
> +      an intermediate FPGA address. The second stage of address translation
> +      occurs between the FPGA address and the CPU address. Use this property
> +      in conjunction with the dma-ranges property to divide the address
> +      translation into these two stages.
> +
> +      If this property is present, one entry is required per dma-range. This is so
> +      FPGA designers can choose to route different address ranges through different
> +      Fabric Interface Controllers and other logic as they see fit.
> +
> +      If this property is not present, the entire address translation
> +      in any dma-ranges property is attempted by the root port driver via its
> +      inbound address translation tables.
> +
> +      Each element in this property has three components. The first is a
> +      PCIe address, the second is an FPGA address, and the third is a size.
> +      These properties may be 32 or 64 bit values.
> +
> +      In operation, the driver will expect a one-to-one correspondance between
> +      dma-range properties and this property.  For each pair of dma-range and
> +      inbound-fabric-translation-range properties, the root port driver will
> +      subtract the FPGA address in this property from the CPU address in the
> +      corresponding dma-range property and use the remainder to program its
> +      inbound address translation tables.
> +
> +      From each dma-range, take its PCIe address and size - these are the PCIe
> +      address & size for the element. The FPGA address is derived from a given
> +      FPGA fabric design and is the address delivered by that FPGA fabric
> +      design to the Core Complex. For a trivial configuration, this property
> +      is unlikely to be required (i.e. no fabric translation on the inbound
> +      interface).  Otherwise, it is tightly coupled with the inbound data path
> +      configured in the FPGA fabric between the root port and the Core Complex.
> +      It is expected that more than one translation range may be added to
> +      an FPGA fabric design, e.g. to deliver data to cached or non-cached
> +      DDR.
> +
> +      For more information on the tables, see Section 1.3.3,
> +      "PCIe/AXI4 Address Translation" of the PolarFire SoC PCIe User Guide:
> +      https://www.microsemi.com/document-portal/doc_download/1245812-polarfire-fpga-and-polarfire-soc-fpga-pci-express-user-guide
> +
> +    items:
> +      minItems: 4
> +      maxItems: 7
> +
>    msi-controller:
>      description: Identifies the node as an MSI controller.
>
> --
> 2.25.1
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 1/4] dt-bindings: PCI: microchip: add fabric address translation properties

[Daire.McNamara]

Thanks Rob for taking the time to get back to me.  Yes, I thought 2
levels of translation might be the way to go. If you could point me to
a gadget that has 2-level translation working, that'd be great

On Fri, 2022-09-02 at 11:28 -0500, Rob Herring wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Fri, Sep 2, 2022 at 9:22 AM <daire.mcnamara@microchip.com> wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > On PolarFire SoC both in- & out-bound address translations occur in
> > two
> > stages. The specific translations are tightly coupled to the FPGA
> > designs and supplement the {dma-,}ranges properties. The first
> > stage of
> > the translation is done by the FPGA fabric & the second by the root
> > port.
> > Add two properties so that the translation tables in the root
> > port's
> > bridge layer can be configured to account for the translation done
> > by
> > the FPGA fabric.
> 
> I'm skeptical that ranges/dma-ranges can't handle what you need.
> Anything in this area is going to need justification 'ranges doesn't
> work because x, y, z...'.
Cool.
> 
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> > ---
> >  .../bindings/pci/microchip,pcie-host.yaml     | 107
> > ++++++++++++++++++
> >  1 file changed, 107 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-
> > host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-
> > host.yaml
> > index 23d95c65acff..29bb1fe99a2e 100644
> > --- a/Documentation/devicetree/bindings/pci/microchip,pcie-
> > host.yaml
> > +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-
> > host.yaml
> > @@ -71,6 +71,113 @@ properties:
> >      minItems: 1
> >      maxItems: 6
> > 
> > +  microchip,outbound-fabric-translation-ranges:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +    minItems: 1
> > +    maxItems: 32
> > +    description: |
> > +      The CPU-to-PCIe (outbound) address translation takes place
> > in two stages.
> > +      Depending on the FPGA bitstream, the outbound address
> > translation tables
> > +      in the PCIe root port's bridge layer will need to be
> > configured to account
> > +      for only its part of the overall outbound address
> > translation.
> > +
> > +      The first stage of outbound address translation occurs
> > between the CPU address
> > +      and an intermediate "FPGA address". The second stage of
> > outbound address
> > +      translation occurs between this FPGA address and the PCIe
> > address. Use this
> > +      property, in conjunction with the ranges property, to divide
> > the overall
> > +      address translation into these two stages so that the PCIe
> > address
> > +      translation tables can be correctly configured.
> 
> Sounds like you need 2 levels of ranges/dma-ranges.
> 
> / {
>     fpga-bus {
>         ranges = ...
>         dma-ranges = ...
>         pcie@... {
>             ranges = ...
>             dma-ranges = ...
>         };
>     };
> };
> 
Very possibly! I'd prefer that approach, tbh, if I can get it working. 
I'll go back and try 2 levels again.


> > +      If this property is present, one entry is required per
> > range. This is so
> > +      FPGA designers can choose to route different address ranges
> > through different
> > +      Fabric Interface Controllers and other logic as they see
> > fit.
> > +
> > +      If this property is not present, the entire address
> > translation
> > +      in any ranges property is attempted by the root port driver
> > via its outbound
> > +      address translation tables.
> > +
> > +      Each element in this property has three components. The
> > first is a
> > +      PCIe address, the second is an FPGA address, and the third
> > is a size.
> > +      These properties may be 32 or 64 bit values.
> > +
> > +      In operation, the driver will expect a one-to-one
> > correspondance between
> > +      range properties and this property.  For each pair of range
> > and
> > +      outbound-fabric-translation-range properties, the root port
> > driver will
> > +      subtract the FPGA address in this property from the CPU
> > address in the
> > +      corresponding range property and use the remainder to
> > program its
> > +      outbound address translation tables.
> > +
> > +      For each range, take its PCIe address and size - these are
> > the PCIe
> > +      address & size for the element. The FPGA address is derived
> > from a given
> > +      FPGA fabric design and is the address delivered by that FPGA
> > fabric
> > +      design to the Core Complex. For a trivial configuration, it
> > is likely to be the
> > +      lower 32 bits of the PCIe address in the range property and
> > the upper
> > +      bits of the base address of the Fabric Interface Controller
> > the design uses.
> > +      Otherwise, it is tightly coupled with the data path
> > configured in the
> > +      FPGA fabric between the root port and the Core Complex.
> > +
> > +      For more information on the tables, see Section 1.3.3,
> > +      "PCIe/AXI4 Address Translation" of the PolarFire SoC PCIe
> > User Guide:
> > +      
> > https://www.microsemi.com/document-portal/doc_download/1245812-polarfire-fpga-and-polarfire-soc-fpga-pci-express-user-guide
> > +
> > +    items:
> > +      minItems: 3
> > +      maxItems: 6
> > +
> > +  microchip,inbound-fabric-translation-ranges:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +    minItems: 1
> > +    maxItems: 32
> > +    description: |
> > +      The PCIe-to-CPU (inbound) address translation takes place in
> > two stages.
> > +      Depending on the FPGA bitstream, the inbound address
> > translation tables
> > +      in the PCIe root port's bridge layer will need to be
> > configured to account
> > +      for only its part of the overall inbound address
> > translation.
> > +
> > +      The first stage of address translation occurs between the
> > PCIe address and
> > +      an intermediate FPGA address. The second stage of address
> > translation
> > +      occurs between the FPGA address and the CPU address. Use
> > this property
> > +      in conjunction with the dma-ranges property to divide the
> > address
> > +      translation into these two stages.
> > +
> > +      If this property is present, one entry is required per dma-
> > range. This is so
> > +      FPGA designers can choose to route different address ranges
> > through different
> > +      Fabric Interface Controllers and other logic as they see
> > fit.
> > +
> > +      If this property is not present, the entire address
> > translation
> > +      in any dma-ranges property is attempted by the root port
> > driver via its
> > +      inbound address translation tables.
> > +
> > +      Each element in this property has three components. The
> > first is a
> > +      PCIe address, the second is an FPGA address, and the third
> > is a size.
> > +      These properties may be 32 or 64 bit values.
> > +
> > +      In operation, the driver will expect a one-to-one
> > correspondance between
> > +      dma-range properties and this property.  For each pair of
> > dma-range and
> > +      inbound-fabric-translation-range properties, the root port
> > driver will
> > +      subtract the FPGA address in this property from the CPU
> > address in the
> > +      corresponding dma-range property and use the remainder to
> > program its
> > +      inbound address translation tables.
> > +
> > +      From each dma-range, take its PCIe address and size - these
> > are the PCIe
> > +      address & size for the element. The FPGA address is derived
> > from a given
> > +      FPGA fabric design and is the address delivered by that FPGA
> > fabric
> > +      design to the Core Complex. For a trivial configuration,
> > this property
> > +      is unlikely to be required (i.e. no fabric translation on
> > the inbound
> > +      interface).  Otherwise, it is tightly coupled with the
> > inbound data path
> > +      configured in the FPGA fabric between the root port and the
> > Core Complex.
> > +      It is expected that more than one translation range may be
> > added to
> > +      an FPGA fabric design, e.g. to deliver data to cached or
> > non-cached
> > +      DDR.
> > +
> > +      For more information on the tables, see Section 1.3.3,
> > +      "PCIe/AXI4 Address Translation" of the PolarFire SoC PCIe
> > User Guide:
> > +      
> > https://www.microsemi.com/document-portal/doc_download/1245812-polarfire-fpga-and-polarfire-soc-fpga-pci-express-user-guide
> > +
> > +    items:
> > +      minItems: 4
> > +      maxItems: 7
> > +
> >    msi-controller:
> >      description: Identifies the node as an MSI controller.
> > 
> > --
> > 2.25.1
> > 
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 1/4] dt-bindings: PCI: microchip: add fabric address translation properties

[Daire.McNamara]

On Fri, 2022-09-02 at 11:28 -0500, Rob Herring wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Sep 2, 2022 at 9:22 AM <daire.mcnamara@microchip.com> wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > On PolarFire SoC both in- & out-bound address translations occur in two
> > stages. The specific translations are tightly coupled to the FPGA
> > designs and supplement the {dma-,}ranges properties. The first stage of
> > the translation is done by the FPGA fabric & the second by the root
> > port.
> > Add two properties so that the translation tables in the root port's
> > bridge layer can be configured to account for the translation done by
> > the FPGA fabric.
> 
> I'm skeptical that ranges/dma-ranges can't handle what you need.
> Anything in this area is going to need justification 'ranges doesn't
> work because x, y, z...'.
> 
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> > ---
> >  .../bindings/pci/microchip,pcie-host.yaml     | 107 ++++++++++++++++++
> >  1 file changed, 107 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> > index 23d95c65acff..29bb1fe99a2e 100644
> > --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> > +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> > @@ -71,6 +71,113 @@ properties:
> >      minItems: 1
> >      maxItems: 6
> > 
> > +  microchip,outbound-fabric-translation-ranges:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +    minItems: 1
> > +    maxItems: 32
> > +    description: |
> > +      The CPU-to-PCIe (outbound) address translation takes place in two stages.
> > +      Depending on the FPGA bitstream, the outbound address translation tables
> > +      in the PCIe root port's bridge layer will need to be configured to account
> > +      for only its part of the overall outbound address translation.
> > +
> > +      The first stage of outbound address translation occurs between the CPU address
> > +      and an intermediate "FPGA address". The second stage of outbound address
> > +      translation occurs between this FPGA address and the PCIe address. Use this
> > +      property, in conjunction with the ranges property, to divide the overall
> > +      address translation into these two stages so that the PCIe address
> > +      translation tables can be correctly configured.
> 
> Sounds like you need 2 levels of ranges/dma-ranges.
> 
> / {
>     fpga-bus {
>         ranges = ...
>         dma-ranges = ...
>         pcie@... {
>             ranges = ...
>             dma-ranges = ...
>         };
>     };
> };
Thanks a million for looking at this! Very much appreciated.

So, this is what I tried.  I've cut down the dts I used to what I think is the minimum
fragment to discuss the issue I'm facing.

So, I replaced this stanza:

pcie: pcie@3000000000 {
    ...
    reg = <0x30 0x0 0x0 0x8000000>, <0x0 0x43000000 0x0 0x10000>;
    reg-names = "cfg", "apb";
    ranges = <0x0000000 0x0 0x0000000 0x30 0x0000000 0x0 0x8000000>,
             <0x3000000 0x0 0x8000000 0x30 0x8000000 0x0 0x80000000>;
    ...
};

with this two-level stanza:

fpga_bus: fpga-bus {
    #address-cells = <2>;
    #size-cells = <2>;
    ranges = <0 0 0x30 0 0x40 0>;
    compatible = "simple-bus";
    ...

    pcie: pcie@0 {
        reg = <0x0 0x0 0x0 0x8000000>, <0x0 0x43000000 0x0 0x10000>;
        reg-names = "cfg", "apb";
        ranges = <0x0000000 0x0 0x0000000 0 0x0000000 0x0 0x8000000>,
                 <0x3000000 0x0 0x8000000 0 0x8000000 0x0 0x80000000>;
        ...
    };
};

and I ran into two problems:
1) the ranges presented to the driver via  resource_list_for_each_entry(entry, &bridge->windows) 
   were unchanged. The start and end of both resources were still in 0x30'0000'0000 space, 
   not 0x0000'0000 as I'd hoped. The two levels of range had been amalgamated before 
   presentation to the rootport driver, so my initial problem was unchanged ...

2) a new issue cropped up. While the 'cfg' register property is in 0x30'0000'0000 space, 
   the 'abp' interface is actually delivered over a separate FIC and is in a 0x4000'0000 
   memory space. In the two-level stanza, it was now being provided to the rootport 
   driver at a base of 0x30'4000'0000 which is incorrect. This is very typical for 
   designers to route abp over a different FIC to axi. 

I hope I'm making the issues clear.

Any suggestions welcome!

> 
> > +
> > +      If this property is present, one entry is required per range. This is so
> > +      FPGA designers can choose to route different address ranges through different
> > +      Fabric Interface Controllers and other logic as they see fit.
> > +
> > +      If this property is not present, the entire address translation
> > +      in any ranges property is attempted by the root port driver via its outbound
> > +      address translation tables.
> > +
> > +      Each element in this property has three components. The first is a
> > +      PCIe address, the second is an FPGA address, and the third is a size.
> > +      These properties may be 32 or 64 bit values.
> > +
> > +      In operation, the driver will expect a one-to-one correspondance between
> > +      range properties and this property.  For each pair of range and
> > +      outbound-fabric-translation-range properties, the root port driver will
> > +      subtract the FPGA address in this property from the CPU address in the
> > +      corresponding range property and use the remainder to program its
> > +      outbound address translation tables.
> > +
> > +      For each range, take its PCIe address and size - these are the PCIe
> > +      address & size for the element. The FPGA address is derived from a given
> > +      FPGA fabric design and is the address delivered by that FPGA fabric
> > +      design to the Core Complex. For a trivial configuration, it is likely to be the
> > +      lower 32 bits of the PCIe address in the range property and the upper
> > +      bits of the base address of the Fabric Interface Controller the design uses.
> > +      Otherwise, it is tightly coupled with the data path configured in the
> > +      FPGA fabric between the root port and the Core Complex.
> > +
> > +      For more information on the tables, see Section 1.3.3,
> > +      "PCIe/AXI4 Address Translation" of the PolarFire SoC PCIe User Guide:
> > +      https://www.microsemi.com/document-portal/doc_download/1245812-polarfire-fpga-and-polarfire-soc-fpga-pci-express-user-guide
> > +
> > +    items:
> > +      minItems: 3
> > +      maxItems: 6
> > +
> > +  microchip,inbound-fabric-translation-ranges:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +    minItems: 1
> > +    maxItems: 32
> > +    description: |
> > +      The PCIe-to-CPU (inbound) address translation takes place in two stages.
> > +      Depending on the FPGA bitstream, the inbound address translation tables
> > +      in the PCIe root port's bridge layer will need to be configured to account
> > +      for only its part of the overall inbound address translation.
> > +
> > +      The first stage of address translation occurs between the PCIe address and
> > +      an intermediate FPGA address. The second stage of address translation
> > +      occurs between the FPGA address and the CPU address. Use this property
> > +      in conjunction with the dma-ranges property to divide the address
> > +      translation into these two stages.
> > +
> > +      If this property is present, one entry is required per dma-range. This is so
> > +      FPGA designers can choose to route different address ranges through different
> > +      Fabric Interface Controllers and other logic as they see fit.
> > +
> > +      If this property is not present, the entire address translation
> > +      in any dma-ranges property is attempted by the root port driver via its
> > +      inbound address translation tables.
> > +
> > +      Each element in this property has three components. The first is a
> > +      PCIe address, the second is an FPGA address, and the third is a size.
> > +      These properties may be 32 or 64 bit values.
> > +
> > +      In operation, the driver will expect a one-to-one correspondance between
> > +      dma-range properties and this property.  For each pair of dma-range and
> > +      inbound-fabric-translation-range properties, the root port driver will
> > +      subtract the FPGA address in this property from the CPU address in the
> > +      corresponding dma-range property and use the remainder to program its
> > +      inbound address translation tables.
> > +
> > +      From each dma-range, take its PCIe address and size - these are the PCIe
> > +      address & size for the element. The FPGA address is derived from a given
> > +      FPGA fabric design and is the address delivered by that FPGA fabric
> > +      design to the Core Complex. For a trivial configuration, this property
> > +      is unlikely to be required (i.e. no fabric translation on the inbound
> > +      interface).  Otherwise, it is tightly coupled with the inbound data path
> > +      configured in the FPGA fabric between the root port and the Core Complex.
> > +      It is expected that more than one translation range may be added to
> > +      an FPGA fabric design, e.g. to deliver data to cached or non-cached
> > +      DDR.
> > +
> > +      For more information on the tables, see Section 1.3.3,
> > +      "PCIe/AXI4 Address Translation" of the PolarFire SoC PCIe User Guide:
> > +      https://www.microsemi.com/document-portal/doc_download/1245812-polarfire-fpga-and-polarfire-soc-fpga-pci-express-user-guide
> > +
> > +    items:
> > +      minItems: 4
> > +      maxItems: 7
> > +
> >    msi-controller:
> >      description: Identifies the node as an MSI controller.
> > 
> > --
> > 2.25.1
> > 
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 1/4] dt-bindings: PCI: microchip: add fabric address translation properties

[Rob Herring]

On Mon, Sep 05, 2022 at 02:54:07PM +0000, Daire.McNamara@microchip.com wrote:
> On Fri, 2022-09-02 at 11:28 -0500, Rob Herring wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Fri, Sep 2, 2022 at 9:22 AM <daire.mcnamara@microchip.com> wrote:
> > > From: Conor Dooley <conor.dooley@microchip.com>
> > > 
> > > On PolarFire SoC both in- & out-bound address translations occur in two
> > > stages. The specific translations are tightly coupled to the FPGA
> > > designs and supplement the {dma-,}ranges properties. The first stage of
> > > the translation is done by the FPGA fabric & the second by the root
> > > port.
> > > Add two properties so that the translation tables in the root port's
> > > bridge layer can be configured to account for the translation done by
> > > the FPGA fabric.
> > 
> > I'm skeptical that ranges/dma-ranges can't handle what you need.
> > Anything in this area is going to need justification 'ranges doesn't
> > work because x, y, z...'.
> > 
> > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > > Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> > > ---
> > >  .../bindings/pci/microchip,pcie-host.yaml     | 107 ++++++++++++++++++
> > >  1 file changed, 107 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> > > index 23d95c65acff..29bb1fe99a2e 100644
> > > --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> > > @@ -71,6 +71,113 @@ properties:
> > >      minItems: 1
> > >      maxItems: 6
> > > 
> > > +  microchip,outbound-fabric-translation-ranges:
> > > +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > > +    minItems: 1
> > > +    maxItems: 32
> > > +    description: |
> > > +      The CPU-to-PCIe (outbound) address translation takes place in two stages.
> > > +      Depending on the FPGA bitstream, the outbound address translation tables
> > > +      in the PCIe root port's bridge layer will need to be configured to account
> > > +      for only its part of the overall outbound address translation.
> > > +
> > > +      The first stage of outbound address translation occurs between the CPU address
> > > +      and an intermediate "FPGA address". The second stage of outbound address
> > > +      translation occurs between this FPGA address and the PCIe address. Use this
> > > +      property, in conjunction with the ranges property, to divide the overall
> > > +      address translation into these two stages so that the PCIe address
> > > +      translation tables can be correctly configured.
> > 
> > Sounds like you need 2 levels of ranges/dma-ranges.
> > 
> > / {
> >     fpga-bus {
> >         ranges = ...
> >         dma-ranges = ...
> >         pcie@... {
> >             ranges = ...
> >             dma-ranges = ...
> >         };
> >     };
> > };
> Thanks a million for looking at this! Very much appreciated.
> 
> So, this is what I tried.  I've cut down the dts I used to what I think is the minimum
> fragment to discuss the issue I'm facing.
> 
> So, I replaced this stanza:
> 
> pcie: pcie@3000000000 {
>     ...
>     reg = <0x30 0x0 0x0 0x8000000>, <0x0 0x43000000 0x0 0x10000>;
>     reg-names = "cfg", "apb";
>     ranges = <0x0000000 0x0 0x0000000 0x30 0x0000000 0x0 0x8000000>,
>              <0x3000000 0x0 0x8000000 0x30 0x8000000 0x0 0x80000000>;
>     ...
> };
> 
> with this two-level stanza:
> 
> fpga_bus: fpga-bus {
>     #address-cells = <2>;
>     #size-cells = <2>;
>     ranges = <0 0 0x30 0 0x40 0>;
>     compatible = "simple-bus";
>     ...
> 
>     pcie: pcie@0 {
>         reg = <0x0 0x0 0x0 0x8000000>, <0x0 0x43000000 0x0 0x10000>;
>         reg-names = "cfg", "apb";
>         ranges = <0x0000000 0x0 0x0000000 0 0x0000000 0x0 0x8000000>,
>                  <0x3000000 0x0 0x8000000 0 0x8000000 0x0 0x80000000>;
>         ...
>     };
> };
> 
> and I ran into two problems:
> 1) the ranges presented to the driver via  resource_list_for_each_entry(entry, &bridge->windows) 
>    were unchanged. The start and end of both resources were still in 0x30'0000'0000 space, 
>    not 0x0000'0000 as I'd hoped. The two levels of range had been amalgamated before 
>    presentation to the rootport driver, so my initial problem was unchanged ...

Yes, that's expected as the translation will walk up parents to root 
node. You will have to get the untranslated values out of 
ranges yourself. If you use the range parsing functions on the parent 
node ranges, you'll get the 0 from of_range.bus_addr.

> 
> 2) a new issue cropped up. While the 'cfg' register property is in 0x30'0000'0000 space, 
>    the 'abp' interface is actually delivered over a separate FIC and is in a 0x4000'0000 
>    memory space. In the two-level stanza, it was now being provided to the rootport 
>    driver at a base of 0x30'4000'0000 which is incorrect. This is very typical for 
>    designers to route abp over a different FIC to axi. 

If the fpga-bus ranges has a 1:1 entry for 0x43000000 child bus then it 
should get translated correctly. Worst case, you may need to define a 
child bus address outside of 0x30_00000000 range that translates back to 
0x43000000.

Rob

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 2/4] riscv: dts: microchip: add fabric address translation properties

[daire.mcnamara]

From: Daire McNamara <daire.mcnamara@microchip.com>

On PolarFire SoC both in- & out-bound address translations occur in two
stages. The specific translations are tightly coupled to the FPGA
designs and supplement the {dma-,}ranges properties. The first stage of
the translation is done by the FPGA fabric & the second by the root
port.
Add outbound address translation information so that the translation
tables in the root port's bridge layer can be configured to account for
the translation done by the FPGA fabric on Icicle Kit reference design.

Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
---
 arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
index 98f04be0dc6b..6839650e7d1b 100644
--- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
+++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
@@ -57,7 +57,11 @@ pcie: pcie@3000000000 {
 		interrupt-map-mask = <0 0 0 7>;
 		clocks = <&fabric_clk1>, <&fabric_clk3>;
 		clock-names = "fic1", "fic3";
-		ranges = <0x3000000 0x0 0x8000000 0x30 0x8000000 0x0 0x80000000>;
+		ranges = <0x0000000 0x0 0x0000000 0x30 0x0000000 0x0 0x8000000>,
+			 <0x3000000 0x0 0x8000000 0x30 0x8000000 0x0 0x80000000>;
+		microchip,outbound-fabric-translation-ranges =
+			 <0x0000000 0x0 0x0000000 0x30 0x0000000 0x0 0x8000000>,
+			 <0x3000000 0x0 0x8000000 0x30 0x8000000 0x0 0x80000000>;
 		dma-ranges = <0x02000000 0x0 0x00000000 0x0 0x00000000 0x1 0x00000000>;
 		msi-parent = <&pcie>;
 		msi-controller;
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 2/4] riscv: dts: microchip: add fabric address translation properties

[Conor.Dooley]

On 02/09/2022 15:22, daire.mcnamara@microchip.com wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> On PolarFire SoC both in- & out-bound address translations occur in two
> stages. The specific translations are tightly coupled to the FPGA
> designs and supplement the {dma-,}ranges properties. The first stage of
> the translation is done by the FPGA fabric & the second by the root
> port.
> Add outbound address translation information so that the translation
> tables in the root port's bridge layer can be configured to account for
> the translation done by the FPGA fabric on Icicle Kit reference design.
> 
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>

As an FYI to the PCI maintainers, I'll take this patch through the
RISC-V tree once everything else is approved as it conflicts with
some other changes that are pending there.

Thanks,
Conor.

> ---
>   arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
> index 98f04be0dc6b..6839650e7d1b 100644
> --- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
> +++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
> @@ -57,7 +57,11 @@ pcie: pcie@3000000000 {
>                  interrupt-map-mask = <0 0 0 7>;
>                  clocks = <&fabric_clk1>, <&fabric_clk3>;
>                  clock-names = "fic1", "fic3";
> -               ranges = <0x3000000 0x0 0x8000000 0x30 0x8000000 0x0 0x80000000>;
> +               ranges = <0x0000000 0x0 0x0000000 0x30 0x0000000 0x0 0x8000000>,
> +                        <0x3000000 0x0 0x8000000 0x30 0x8000000 0x0 0x80000000>;
> +               microchip,outbound-fabric-translation-ranges =
> +                        <0x0000000 0x0 0x0000000 0x30 0x0000000 0x0 0x8000000>,
> +                        <0x3000000 0x0 0x8000000 0x30 0x8000000 0x0 0x80000000>;
>                  dma-ranges = <0x02000000 0x0 0x00000000 0x0 0x00000000 0x1 0x00000000>;
>                  msi-parent = <&pcie>;
>                  msi-controller;
> --
> 2.25.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 3/4] PCI: microchip: add fabric address translation properties

[daire.mcnamara]

From: Daire McNamara <daire.mcnamara@microchip.com>

On PolarFire SoC both in- & out-bound address translations occur in two
stages. The specific translations are tightly coupled to the FPGA
designs and supplement the {dma-,}ranges properties. The first stage of
the translation is done by the FPGA fabric & the second by the root
port.
Use outbound address translation information so that the translation
tables in the root port's bridge layer can be configured to account for
any translation done by the FPGA fabric, for example,  on Icicle Kit
reference design.

Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
---
 drivers/pci/controller/pcie-microchip-host.c | 59 +++++++++++++++++---
 1 file changed, 52 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
index 7263d175b5ad..d78745eaa4b4 100644
--- a/drivers/pci/controller/pcie-microchip-host.c
+++ b/drivers/pci/controller/pcie-microchip-host.c
@@ -269,6 +269,8 @@ struct mc_pcie {
 	struct irq_domain *event_domain;
 	raw_spinlock_t lock;
 	struct mc_msi msi;
+	u32 num_outbound_ranges;
+	u64 outbound_range_adjustments[32];
 };
 
 struct cause {
@@ -964,6 +966,37 @@ static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index,
 	writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR);
 }
 
+static void mc_pcie_parse_outbound_range_adjustments(struct mc_pcie *port, struct device_node *np)
+{
+	const __be32 *range;
+	int range_len, num_ranges, range_size, i;
+
+	range = of_get_property(np, "microchip,outbound-fabric-translation-ranges", &range_len);
+	if (!range)
+		return;
+
+	num_ranges = of_n_addr_cells(np);
+	range_size = range_len / sizeof(__be32) / num_ranges;
+
+	for (i = 0; i < num_ranges; i++, range += range_size) {
+		u64 pcieaddr = of_read_number(range + 1, 2);
+		u64 cpuaddr = of_read_number(range + 3, 2);
+
+		port->outbound_range_adjustments[i] = cpuaddr - pcieaddr;
+		port->num_outbound_ranges++;
+	}
+}
+
+static inline u64 mc_pcie_adjust_axi(struct mc_pcie *port, int index, u64 axi_addr)
+{
+	u64 offset = 0;
+
+	if (index < port->num_outbound_ranges)
+		offset = port->outbound_range_adjustments[index];
+
+	return axi_addr - offset;
+}
+
 static int mc_pcie_setup_windows(struct platform_device *pdev,
 				 struct mc_pcie *port)
 {
@@ -971,14 +1004,28 @@ static int mc_pcie_setup_windows(struct platform_device *pdev,
 		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
 	struct pci_host_bridge *bridge = platform_get_drvdata(pdev);
 	struct resource_entry *entry;
+	u64 axi_addr;
 	u64 pci_addr;
-	u32 index = 1;
+	u32 index = 0;
+	u32 num_outbound_ranges = 0;
+
+	resource_list_for_each_entry(entry, &bridge->windows) {
+		if (resource_type(entry->res) == IORESOURCE_MEM || resource_type(entry->res) == 0)
+			num_outbound_ranges++;
+	}
+
+	if (port->num_outbound_ranges && port->num_outbound_ranges != num_outbound_ranges) {
+		dev_err(&pdev->dev, "Mismatches in outbound range adjustment\n");
+		return -EINVAL;
+	}
 
 	resource_list_for_each_entry(entry, &bridge->windows) {
-		if (resource_type(entry->res) == IORESOURCE_MEM) {
+		if (resource_type(entry->res) == IORESOURCE_MEM || resource_type(entry->res) == 0) {
+			axi_addr = entry->res->start;
+			axi_addr = mc_pcie_adjust_axi(port, index, axi_addr);
 			pci_addr = entry->res->start - entry->offset;
 			mc_pcie_setup_window(bridge_base_addr, index,
-					     entry->res->start, pci_addr,
+					     axi_addr, pci_addr,
 					     resource_size(entry->res));
 			index++;
 		}
@@ -1005,6 +1052,8 @@ static int mc_platform_init(struct pci_config_window *cfg)
 		return -ENOMEM;
 	port->dev = dev;
 
+	mc_pcie_parse_outbound_range_adjustments(port, dev->of_node);
+
 	ret = mc_pcie_init_clks(dev);
 	if (ret) {
 		dev_err(dev, "failed to get clock resources, error %d\n", ret);
@@ -1099,10 +1148,6 @@ static int mc_platform_init(struct pci_config_window *cfg)
 	writel_relaxed(0, bridge_base_addr + IMASK_HOST);
 	writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_HOST);
 
-	/* Configure Address Translation Table 0 for PCIe config space */
-	mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start & 0xffffffff,
-			     cfg->res.start, resource_size(&cfg->res));
-
 	return mc_pcie_setup_windows(pdev, port);
 }
 
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 3/4] PCI: microchip: add fabric address translation properties

[Bjorn Helgaas]

On Fri, Sep 02, 2022 at 03:22:01PM +0100, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> On PolarFire SoC both in- & out-bound address translations occur in two
> stages. The specific translations are tightly coupled to the FPGA
> designs and supplement the {dma-,}ranges properties. The first stage of
> the translation is done by the FPGA fabric & the second by the root
> port.
> Use outbound address translation information so that the translation
> tables in the root port's bridge layer can be configured to account for
> any translation done by the FPGA fabric, for example,  on Icicle Kit
> reference design.

Can you please:

  - Make your subject follow previous convention, i.e., at least
    capitalize "Add".

  - Add a blank line between paragraphs.  Patch 2/4 also lacks this
    blank line.  Without the separator, it's just confusing because I
    can't tell whether it's supposed to be a single paragraph that you
    forgot to wrap correctly, or two paragraphs.

> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> ---
>  drivers/pci/controller/pcie-microchip-host.c | 59 +++++++++++++++++---
>  1 file changed, 52 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> index 7263d175b5ad..d78745eaa4b4 100644
> --- a/drivers/pci/controller/pcie-microchip-host.c
> +++ b/drivers/pci/controller/pcie-microchip-host.c
> @@ -269,6 +269,8 @@ struct mc_pcie {
>  	struct irq_domain *event_domain;
>  	raw_spinlock_t lock;
>  	struct mc_msi msi;
> +	u32 num_outbound_ranges;
> +	u64 outbound_range_adjustments[32];
>  };
>  
>  struct cause {
> @@ -964,6 +966,37 @@ static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index,
>  	writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR);
>  }
>  
> +static void mc_pcie_parse_outbound_range_adjustments(struct mc_pcie *port, struct device_node *np)

Wrap to fit in 80 columns like the rest of the file.

> +{
> +	const __be32 *range;
> +	int range_len, num_ranges, range_size, i;
> +
> +	range = of_get_property(np, "microchip,outbound-fabric-translation-ranges", &range_len);
> +	if (!range)
> +		return;
> +
> +	num_ranges = of_n_addr_cells(np);
> +	range_size = range_len / sizeof(__be32) / num_ranges;
> +
> +	for (i = 0; i < num_ranges; i++, range += range_size) {
> +		u64 pcieaddr = of_read_number(range + 1, 2);
> +		u64 cpuaddr = of_read_number(range + 3, 2);
> +
> +		port->outbound_range_adjustments[i] = cpuaddr - pcieaddr;
> +		port->num_outbound_ranges++;
> +	}
> +}
> +
> +static inline u64 mc_pcie_adjust_axi(struct mc_pcie *port, int index, u64 axi_addr)

No need for this to be inline; it's not a performance path so the
"inline" annotation is just clutter and makes the line too long.

> +{
> +	u64 offset = 0;
> +
> +	if (index < port->num_outbound_ranges)
> +		offset = port->outbound_range_adjustments[index];
> +
> +	return axi_addr - offset;

  if (index < port->num_outbound_ranges)
    return axi_addr - port->outbound_range_adjustments[index];

  return axi_addr;

> +}
> +
>  static int mc_pcie_setup_windows(struct platform_device *pdev,
>  				 struct mc_pcie *port)
>  {
> @@ -971,14 +1004,28 @@ static int mc_pcie_setup_windows(struct platform_device *pdev,
>  		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
>  	struct pci_host_bridge *bridge = platform_get_drvdata(pdev);
>  	struct resource_entry *entry;
> +	u64 axi_addr;
>  	u64 pci_addr;
> -	u32 index = 1;
> +	u32 index = 0;
> +	u32 num_outbound_ranges = 0;
> +
> +	resource_list_for_each_entry(entry, &bridge->windows) {
> +		if (resource_type(entry->res) == IORESOURCE_MEM || resource_type(entry->res) == 0)

Rewrap.

> +			num_outbound_ranges++;
> +	}
> +
> +	if (port->num_outbound_ranges && port->num_outbound_ranges != num_outbound_ranges) {

Ditto.

> +		dev_err(&pdev->dev, "Mismatches in outbound range adjustment\n");
> +		return -EINVAL;
> +	}
>  
>  	resource_list_for_each_entry(entry, &bridge->windows) {
> -		if (resource_type(entry->res) == IORESOURCE_MEM) {
> +		if (resource_type(entry->res) == IORESOURCE_MEM || resource_type(entry->res) == 0) {

Ditto.

I guess "resource_type() == 0" means config space?  I assume these
entries came from devm_of_pci_get_host_bridge_resources()?  From
gen_pci_init(), I guess there's an assumption that the resource at
index 0 is ECAM space?

> +			axi_addr = entry->res->start;
> +			axi_addr = mc_pcie_adjust_axi(port, index, axi_addr);

How does this address adjustment work given that
pci_host_common_probe() has already called gen_pci_init() to map the
config space?

Hopefully you can use Rob's suggestion to just use two levels of
ranges instead.

>  			pci_addr = entry->res->start - entry->offset;
>  			mc_pcie_setup_window(bridge_base_addr, index,
> -					     entry->res->start, pci_addr,
> +					     axi_addr, pci_addr,
>  					     resource_size(entry->res));
>  			index++;
>  		}
> @@ -1005,6 +1052,8 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  		return -ENOMEM;
>  	port->dev = dev;
>  
> +	mc_pcie_parse_outbound_range_adjustments(port, dev->of_node);
> +
>  	ret = mc_pcie_init_clks(dev);
>  	if (ret) {
>  		dev_err(dev, "failed to get clock resources, error %d\n", ret);
> @@ -1099,10 +1148,6 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  	writel_relaxed(0, bridge_base_addr + IMASK_HOST);
>  	writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_HOST);
>  
> -	/* Configure Address Translation Table 0 for PCIe config space */
> -	mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start & 0xffffffff,
> -			     cfg->res.start, resource_size(&cfg->res));
> -
>  	return mc_pcie_setup_windows(pdev, port);
>  }

Not specifically related to *this* patch, but microchip uses the
pci_ecam_ops.init() method to do a whole bunch of init completely
unrelated to ECAM, which makes things really hard to follow.

It would be more readable to have an mc_pcie_probe() that does the
mc-specific initialization and calls pci_host_common_probe() to do the
generic stuff.  This is what apple_pcie_probe() does.

Bjorn

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 3/4] PCI: microchip: add fabric address translation properties

[Daire.McNamara]

Thanks Bjorn for the feedback.  I'll follow up Rob's suggestion and see
how I get on!

On Fri, 2022-09-02 at 11:49 -0500, Bjorn Helgaas wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Fri, Sep 02, 2022 at 03:22:01PM +0100, 
> daire.mcnamara@microchip.com wrote:
> > From: Daire McNamara <daire.mcnamara@microchip.com>
> > 
> > On PolarFire SoC both in- & out-bound address translations occur in
> > two
> > stages. The specific translations are tightly coupled to the FPGA
> > designs and supplement the {dma-,}ranges properties. The first
> > stage of
> > the translation is done by the FPGA fabric & the second by the root
> > port.
> > Use outbound address translation information so that the
> > translation
> > tables in the root port's bridge layer can be configured to account
> > for
> > any translation done by the FPGA fabric, for example,  on Icicle
> > Kit
> > reference design.
> 
> Can you please:
> 
>   - Make your subject follow previous convention, i.e., at least
>     capitalize "Add".
> 
>   - Add a blank line between paragraphs.  Patch 2/4 also lacks this
>     blank line.  Without the separator, it's just confusing because I
>     can't tell whether it's supposed to be a single paragraph that
> you
>     forgot to wrap correctly, or two paragraphs.
> 
> > Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> > ---
> >  drivers/pci/controller/pcie-microchip-host.c | 59
> > +++++++++++++++++---
> >  1 file changed, 52 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-microchip-host.c
> > b/drivers/pci/controller/pcie-microchip-host.c
> > index 7263d175b5ad..d78745eaa4b4 100644
> > --- a/drivers/pci/controller/pcie-microchip-host.c
> > +++ b/drivers/pci/controller/pcie-microchip-host.c
> > @@ -269,6 +269,8 @@ struct mc_pcie {
> >       struct irq_domain *event_domain;
> >       raw_spinlock_t lock;
> >       struct mc_msi msi;
> > +     u32 num_outbound_ranges;
> > +     u64 outbound_range_adjustments[32];
> >  };
> > 
> >  struct cause {
> > @@ -964,6 +966,37 @@ static void mc_pcie_setup_window(void __iomem
> > *bridge_base_addr, u32 index,
> >       writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR);
> >  }
> > 
> > +static void mc_pcie_parse_outbound_range_adjustments(struct
> > mc_pcie *port, struct device_node *np)
> 
> Wrap to fit in 80 columns like the rest of the file.
> 
> > +{
> > +     const __be32 *range;
> > +     int range_len, num_ranges, range_size, i;
> > +
> > +     range = of_get_property(np, "microchip,outbound-fabric-
> > translation-ranges", &range_len);
> > +     if (!range)
> > +             return;
> > +
> > +     num_ranges = of_n_addr_cells(np);
> > +     range_size = range_len / sizeof(__be32) / num_ranges;
> > +
> > +     for (i = 0; i < num_ranges; i++, range += range_size) {
> > +             u64 pcieaddr = of_read_number(range + 1, 2);
> > +             u64 cpuaddr = of_read_number(range + 3, 2);
> > +
> > +             port->outbound_range_adjustments[i] = cpuaddr -
> > pcieaddr;
> > +             port->num_outbound_ranges++;
> > +     }
> > +}
> > +
> > +static inline u64 mc_pcie_adjust_axi(struct mc_pcie *port, int
> > index, u64 axi_addr)
> 
> No need for this to be inline; it's not a performance path so the
> "inline" annotation is just clutter and makes the line too long.
> 
> > +{
> > +     u64 offset = 0;
> > +
> > +     if (index < port->num_outbound_ranges)
> > +             offset = port->outbound_range_adjustments[index];
> > +
> > +     return axi_addr - offset;
> 
>   if (index < port->num_outbound_ranges)
>     return axi_addr - port->outbound_range_adjustments[index];
> 
>   return axi_addr;
> 
> > +}
> > +
> >  static int mc_pcie_setup_windows(struct platform_device *pdev,
> >                                struct mc_pcie *port)
> >  {
> > @@ -971,14 +1004,28 @@ static int mc_pcie_setup_windows(struct
> > platform_device *pdev,
> >               port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> >       struct pci_host_bridge *bridge = platform_get_drvdata(pdev);
> >       struct resource_entry *entry;
> > +     u64 axi_addr;
> >       u64 pci_addr;
> > -     u32 index = 1;
> > +     u32 index = 0;
> > +     u32 num_outbound_ranges = 0;
> > +
> > +     resource_list_for_each_entry(entry, &bridge->windows) {
> > +             if (resource_type(entry->res) == IORESOURCE_MEM ||
> > resource_type(entry->res) == 0)
> 
> Rewrap.
> 
> > +                     num_outbound_ranges++;
> > +     }
> > +
> > +     if (port->num_outbound_ranges && port->num_outbound_ranges !=
> > num_outbound_ranges) {
> 
> Ditto.
> 
> > +             dev_err(&pdev->dev, "Mismatches in outbound range
> > adjustment\n");
> > +             return -EINVAL;
> > +     }
> > 
> >       resource_list_for_each_entry(entry, &bridge->windows) {
> > -             if (resource_type(entry->res) == IORESOURCE_MEM) {
> > +             if (resource_type(entry->res) == IORESOURCE_MEM ||
> > resource_type(entry->res) == 0) {
> 
> Ditto.
> 
> I guess "resource_type() == 0" means config space?  I assume these
> entries came from devm_of_pci_get_host_bridge_resources()?  From
> gen_pci_init(), I guess there's an assumption that the resource at
> index 0 is ECAM space?
> 
> > +                     axi_addr = entry->res->start;
> > +                     axi_addr = mc_pcie_adjust_axi(port, index,
> > axi_addr);
> 
> How does this address adjustment work given that
> pci_host_common_probe() has already called gen_pci_init() to map the
> config space?
> 
> Hopefully you can use Rob's suggestion to just use two levels of
> ranges instead.
> 
> >                       pci_addr = entry->res->start - entry->offset;
> >                       mc_pcie_setup_window(bridge_base_addr, index,
> > -                                          entry->res->start,
> > pci_addr,
> > +                                          axi_addr, pci_addr,
> >                                            resource_size(entry-
> > >res));
> >                       index++;
> >               }
> > @@ -1005,6 +1052,8 @@ static int mc_platform_init(struct
> > pci_config_window *cfg)
> >               return -ENOMEM;
> >       port->dev = dev;
> > 
> > +     mc_pcie_parse_outbound_range_adjustments(port, dev->of_node);
> > +
> >       ret = mc_pcie_init_clks(dev);
> >       if (ret) {
> >               dev_err(dev, "failed to get clock resources, error
> > %d\n", ret);
> > @@ -1099,10 +1148,6 @@ static int mc_platform_init(struct
> > pci_config_window *cfg)
> >       writel_relaxed(0, bridge_base_addr + IMASK_HOST);
> >       writel_relaxed(GENMASK(31, 0), bridge_base_addr +
> > ISTATUS_HOST);
> > 
> > -     /* Configure Address Translation Table 0 for PCIe config
> > space */
> > -     mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start &
> > 0xffffffff,
> > -                          cfg->res.start, resource_size(&cfg-
> > >res));
> > -
> >       return mc_pcie_setup_windows(pdev, port);
> >  }
> 
> Not specifically related to *this* patch, but microchip uses the
> pci_ecam_ops.init() method to do a whole bunch of init completely
> unrelated to ECAM, which makes things really hard to follow.
> 
> It would be more readable to have an mc_pcie_probe() that does the
> mc-specific initialization and calls pci_host_common_probe() to do
> the
> generic stuff.  This is what apple_pcie_probe() does.
> 
> Bjorn
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 4/4] of: PCI: tidy up logging of ranges containing configuration space type

[daire.mcnamara]

From: Daire McNamara <daire.mcnamara@microchip.com>

PCI ranges can contain addresses where phys.high part can have a type
of 0, signifying 'configuration space'.  Change
devm_of_pci_get_host_bridge_resources() to print 'CFG' instead of 'err'
for a PCI range containing such a 'configuration space' type.

Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
---
 drivers/pci/of.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 196834ed44fe..d782ad8c7dce 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -319,6 +319,8 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
 			range_type = "IO";
 		else if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM)
 			range_type = "MEM";
+		else if ((range.flags & IORESOURCE_TYPE_BITS) == 0)
+			range_type = "CFG";
 		else
 			range_type = "err";
 		dev_info(dev, "  %6s %#012llx..%#012llx -> %#012llx\n",
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 4/4] of: PCI: tidy up logging of ranges containing configuration space type

[Rob Herring]

On Fri, Sep 02, 2022 at 03:22:02PM +0100, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> PCI ranges can contain addresses where phys.high part can have a type
> of 0, signifying 'configuration space'.  Change
> devm_of_pci_get_host_bridge_resources() to print 'CFG' instead of 'err'
> for a PCI range containing such a 'configuration space' type.

Generally, putting config space into ranges is wrong. It should be in 
'reg'

Rob

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv