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

[Rob Herring <robh@kernel.org>]

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