diff mbox series

[v2,1/4] dt-bindings: PCI: dwc: rockchip: Add atu property

Message ID 20231024151014.240695-2-nks@flawful.org (mailing list archive)
State New, archived
Headers show
Series rk3588 PCIe improvements | expand

Commit Message

Niklas Cassel Oct. 24, 2023, 3:10 p.m. UTC 
From: Niklas Cassel <niklas.cassel@wdc.com>

Even though rockchip-dw-pcie.yaml inherits snps,dw-pcie.yaml
using:

allOf:
  - $ref: /schemas/pci/snps,dw-pcie.yaml#

and snps,dw-pcie.yaml does have the atu property defined, in order to be
able to use this property, while still making sure 'make CHECK_DTBS=y'
pass, we need to add this property to rockchip-dw-pcie.yaml.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Conor Dooley Oct. 24, 2023, 4:29 p.m. UTC | #1 
On Tue, Oct 24, 2023 at 05:10:08PM +0200, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Even though rockchip-dw-pcie.yaml inherits snps,dw-pcie.yaml
> using:
> 
> allOf:
>   - $ref: /schemas/pci/snps,dw-pcie.yaml#
> 
> and snps,dw-pcie.yaml does have the atu property defined, in order to be
> able to use this property, while still making sure 'make CHECK_DTBS=y'
> pass, we need to add this property to rockchip-dw-pcie.yaml.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> index 1ae8dcfa072c..229f8608c535 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> @@ -29,16 +29,20 @@ properties:
>            - const: rockchip,rk3568-pcie
>  
>    reg:
> +    minItems: 3
>      items:
>        - description: Data Bus Interface (DBI) registers
>        - description: Rockchip designed configuration registers
>        - description: Config registers
> +      - description: iATU registers

Is this extra register only for the ..88 or for the ..68 and for the
..88 models?

>  
>    reg-names:
> +    minItems: 3
>      items:
>        - const: dbi
>        - const: apb
>        - const: config
> +      - const: atu
>  
>    clocks:
>      minItems: 5
> -- 
> 2.41.0
>
Niklas Cassel Oct. 25, 2023, 8:02 p.m. UTC | #2 
Hello Conor,

On Tue, Oct 24, 2023 at 05:29:28PM +0100, Conor Dooley wrote:
> On Tue, Oct 24, 2023 at 05:10:08PM +0200, Niklas Cassel wrote:
> > From: Niklas Cassel <niklas.cassel@wdc.com>
> > 
> > Even though rockchip-dw-pcie.yaml inherits snps,dw-pcie.yaml
> > using:
> > 
> > allOf:
> >   - $ref: /schemas/pci/snps,dw-pcie.yaml#
> > 
> > and snps,dw-pcie.yaml does have the atu property defined, in order to be
> > able to use this property, while still making sure 'make CHECK_DTBS=y'
> > pass, we need to add this property to rockchip-dw-pcie.yaml.
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> >  Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > index 1ae8dcfa072c..229f8608c535 100644
> > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > @@ -29,16 +29,20 @@ properties:
> >            - const: rockchip,rk3568-pcie
> >  
> >    reg:
> > +    minItems: 3
> >      items:
> >        - description: Data Bus Interface (DBI) registers
> >        - description: Rockchip designed configuration registers
> >        - description: Config registers
> > +      - description: iATU registers
> 
> Is this extra register only for the ..88 or for the ..68 and for the
> ..88 models?

Looking at the rk3568 Technical Reference Manual (TRM):
https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf

The iATU register register range exists for all 3 PCIe controllers
found on the rk3568.

This register range is currently not defined in the rk3568.dtsi, so the driver
will currently use the default register offset (which is correct), but with
the driver fallback register size that is only big enough to cover 8 inbound
and 8 outbound iATUs (internal Address Translation Units).

According to the TRM, all three PCIe controllers found on the rk3568 have
16 inbound iATUs and 16 outbound iATUs, so if someone wants to be able to
make use of all the iATUs on the rk3568, they will need to add "atu" to
rk3568.dtsi.

I don't have the board, so I can't test, but I could add a patch that adds
the "atu" range for the controllers in rk3568, if that makes things easier.


Kind regards,
Niklas
Rob Herring Oct. 26, 2023, 6:20 p.m. UTC | #3 
On Tue, Oct 24, 2023 at 05:10:08PM +0200, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>

The subject says 'atu property', but 'atu' is not a property.

> 
> Even though rockchip-dw-pcie.yaml inherits snps,dw-pcie.yaml
> using:
> 
> allOf:
>   - $ref: /schemas/pci/snps,dw-pcie.yaml#
> 
> and snps,dw-pcie.yaml does have the atu property defined, in order to be
> able to use this property, while still making sure 'make CHECK_DTBS=y'
> pass, we need to add this property to rockchip-dw-pcie.yaml.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> index 1ae8dcfa072c..229f8608c535 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> @@ -29,16 +29,20 @@ properties:
>            - const: rockchip,rk3568-pcie
>  
>    reg:
> +    minItems: 3
>      items:
>        - description: Data Bus Interface (DBI) registers
>        - description: Rockchip designed configuration registers
>        - description: Config registers
> +      - description: iATU registers
>  
>    reg-names:
> +    minItems: 3
>      items:
>        - const: dbi
>        - const: apb
>        - const: config
> +      - const: atu
>  
>    clocks:
>      minItems: 5
> -- 
> 2.41.0
>
Rob Herring Oct. 26, 2023, 6:35 p.m. UTC | #4 
On Wed, Oct 25, 2023 at 08:02:32PM +0000, Niklas Cassel wrote:
> Hello Conor,
> 
> On Tue, Oct 24, 2023 at 05:29:28PM +0100, Conor Dooley wrote:
> > On Tue, Oct 24, 2023 at 05:10:08PM +0200, Niklas Cassel wrote:
> > > From: Niklas Cassel <niklas.cassel@wdc.com>
> > > 
> > > Even though rockchip-dw-pcie.yaml inherits snps,dw-pcie.yaml
> > > using:
> > > 
> > > allOf:
> > >   - $ref: /schemas/pci/snps,dw-pcie.yaml#
> > > 
> > > and snps,dw-pcie.yaml does have the atu property defined, in order to be
> > > able to use this property, while still making sure 'make CHECK_DTBS=y'
> > > pass, we need to add this property to rockchip-dw-pcie.yaml.
> > > 
> > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > > ---
> > >  Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > > index 1ae8dcfa072c..229f8608c535 100644
> > > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > > @@ -29,16 +29,20 @@ properties:
> > >            - const: rockchip,rk3568-pcie
> > >  
> > >    reg:
> > > +    minItems: 3
> > >      items:
> > >        - description: Data Bus Interface (DBI) registers
> > >        - description: Rockchip designed configuration registers
> > >        - description: Config registers
> > > +      - description: iATU registers
> > 
> > Is this extra register only for the ..88 or for the ..68 and for the
> > ..88 models?
> 
> Looking at the rk3568 Technical Reference Manual (TRM):
> https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf
> 
> The iATU register register range exists for all 3 PCIe controllers
> found on the rk3568.
> 
> This register range is currently not defined in the rk3568.dtsi, so the driver
> will currently use the default register offset (which is correct), but with
> the driver fallback register size that is only big enough to cover 8 inbound
> and 8 outbound iATUs (internal Address Translation Units).

We should probably make the driver smarter instead or in addition. We 
have the DBI size, Just make atu_size = dbi_size - DEFAULT_DBI_ATU_OFFSET.

> According to the TRM, all three PCIe controllers found on the rk3568 have
> 16 inbound iATUs and 16 outbound iATUs, so if someone wants to be able to
> make use of all the iATUs on the rk3568, they will need to add "atu" to
> rk3568.dtsi.

At least for host side, the number of regions used is based on ranges. 
You'd be hard pressed to need more than 8. That or no h/w with 16 is 
probably why I said 8 was enough at the time.

Rob
Niklas Cassel Oct. 27, 2023, 4:37 p.m. UTC | #5 
On Fri, Oct 27, 2023 at 10:56:50AM -0500, Rob Herring wrote:
> On Fri, Oct 27, 2023 at 9:35 AM Niklas Cassel <Niklas.Cassel@wdc.com> wrote:
> >
> > Hello Rob,
> >
> > On Thu, Oct 26, 2023 at 01:35:01PM -0500, Rob Herring wrote:
> > > On Wed, Oct 25, 2023 at 08:02:32PM +0000, Niklas Cassel wrote:
> > > > Hello Conor,
> > > >
> > > > On Tue, Oct 24, 2023 at 05:29:28PM +0100, Conor Dooley wrote:
> > > > > On Tue, Oct 24, 2023 at 05:10:08PM +0200, Niklas Cassel wrote:
> > > > > > From: Niklas Cassel <niklas.cassel@wdc.com>
> > > > > >
> > > > > > Even though rockchip-dw-pcie.yaml inherits snps,dw-pcie.yaml
> > > > > > using:
> > > > > >
> > > > > > allOf:
> > > > > >   - $ref: /schemas/pci/snps,dw-pcie.yaml#
> > > > > >
> > > > > > and snps,dw-pcie.yaml does have the atu property defined, in order to be
> > > > > > able to use this property, while still making sure 'make CHECK_DTBS=y'
> > > > > > pass, we need to add this property to rockchip-dw-pcie.yaml.
> > > > > >
> > > > > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml | 4 ++++
> > > > > >  1 file changed, 4 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > > > > > index 1ae8dcfa072c..229f8608c535 100644
> > > > > > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > > > > > @@ -29,16 +29,20 @@ properties:
> > > > > >            - const: rockchip,rk3568-pcie
> > > > > >
> > > > > >    reg:
> > > > > > +    minItems: 3
> > > > > >      items:
> > > > > >        - description: Data Bus Interface (DBI) registers
> > > > > >        - description: Rockchip designed configuration registers
> > > > > >        - description: Config registers
> > > > > > +      - description: iATU registers
> > > > >
> > > > > Is this extra register only for the ..88 or for the ..68 and for the
> > > > > ..88 models?
> > > >
> > > > Looking at the rk3568 Technical Reference Manual (TRM):
> > > > https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf
> > > >
> > > > The iATU register register range exists for all 3 PCIe controllers
> > > > found on the rk3568.
> > > >
> > > > This register range is currently not defined in the rk3568.dtsi, so the driver
> > > > will currently use the default register offset (which is correct), but with
> > > > the driver fallback register size that is only big enough to cover 8 inbound
> > > > and 8 outbound iATUs (internal Address Translation Units).
> > >
> > > We should probably make the driver smarter instead or in addition. We
> > > have the DBI size, Just make atu_size = dbi_size - DEFAULT_DBI_ATU_OFFSET.
> >
> > I though about that, but it seems that some drivers don't use
> > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi")
> >
> > but instead set pci->dbi_base from non-common code, e.g.:
> > drivers/pci/controller/dwc/pci-dra7xx.c:        pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "ep_dbics");
> > drivers/pci/controller/dwc/pci-dra7xx.c:        pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "rc_dbics");
> > drivers/pci/controller/dwc/pci-imx6.c:  pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev, 0, &dbi_base);
> > drivers/pci/controller/dwc/pci-keystone.c:      pci->dbi_base = base;
> > drivers/pci/controller/dwc/pci-layerscape-ep.c: dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
> > drivers/pci/controller/dwc/pci-layerscape-ep.c: pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > drivers/pci/controller/dwc/pci-layerscape.c:    dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
> > drivers/pci/controller/dwc/pci-layerscape.c:    pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > drivers/pci/controller/dwc/pci-meson.c: pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "elbi");
> > drivers/pci/controller/dwc/pcie-al.c:   void __iomem *dbi_base = pcie->dbi_base;
> > drivers/pci/controller/dwc/pcie-al.c:   al_pcie->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> > drivers/pci/controller/dwc/pcie-armada8k.c:     pci->dbi_base = devm_pci_remap_cfg_resource(dev, base);
> > drivers/pci/controller/dwc/pcie-designware.c:           pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
> > drivers/pci/controller/dwc/pcie-histb.c:        pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "rc-dbi");
> > drivers/pci/controller/dwc/pcie-qcom-ep.c:      pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> > drivers/pci/controller/dwc/pcie-tegra194-acpi.c:        pcie_ecam->dbi_base = cfg->win + SZ_512K;
> >
> > So I don't think that we can always get the size of the dbi.
> > And a solution that does not work for all platforms is not
> > that appealing.
> 
> Do I get a chance to respond before you send a new version?
> 
> Does something like the patch below not work for everyone? We could
> store the DBI size as well if we want more than 8 regions to work
> without a 'dbi' nor 'atu' region defined. We shouldn't have new users
> doing that though.

For the drivers listed above, they assign pci->dbi_base in non-common code.

> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> b/drivers/pci/controller/dwc/pcie-designware.c
> index 250cf7f40b85..3dc71ea7fa76 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -105,11 +105,13 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>         struct platform_device *pdev = to_platform_device(pci->dev);
>         struct device_node *np = dev_of_node(pci->dev);
>         struct resource *res;
> +       size_t dbi_size = DEFAULT_DBI_ATU_OFFSET + SZ_4K;
>         int ret;
>

So those drivers will not enter this if-statement:

>         if (!pci->dbi_base) {
>                 res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
>                 pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
> +               dbi_size = resource_size(res);
>                 if (IS_ERR(pci->dbi_base))
>                         return PTR_ERR(pci->dbi_base);
>         }

So for those platforms default size will still be 4k.


But sure, this will avoid the need for some platforms to specify "atu" reg,
so I'm all for it. Could you please send it as a real patch?

If you do, could you please also update the description of the dt binding:
https://github.com/torvalds/linux/blob/v6.6-rc7/Documentation/devicetree/bindings/pci/snps%2Cdw-pcie.yaml#L41-L43

That says that iATU memory region is required if the space is unrolled.


Kind regards,
Niklas
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
index 1ae8dcfa072c..229f8608c535 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
@@ -29,16 +29,20 @@  properties:
           - const: rockchip,rk3568-pcie
 
   reg:
+    minItems: 3
     items:
       - description: Data Bus Interface (DBI) registers
       - description: Rockchip designed configuration registers
       - description: Config registers
+      - description: iATU registers
 
   reg-names:
+    minItems: 3
     items:
       - const: dbi
       - const: apb
       - const: config
+      - const: atu
 
   clocks:
     minItems: 5