Re: [PATCH v2 3/4] dt-bindings: PCI: dwc: rockchip: Add dma properties

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Wed, Oct 25, 2023 at 10:07:02PM +0200, Niklas Cassel wrote:
> Hello Conor,
> 
> On Tue, Oct 24, 2023 at 05:30:22PM +0100, Conor Dooley wrote:
> > On Tue, Oct 24, 2023 at 05:10:10PM +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 dma properties defined, in order to be
> > > able to use these properties, while still making sure 'make CHECK_DTBS=y'
> > > pass, we need to add these properties to rockchip-dw-pcie.yaml.
> > > 
> > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > > ---
> > >  .../bindings/pci/rockchip-dw-pcie.yaml        | 20 +++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > > index 229f8608c535..633f8e0e884f 100644
> > > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > > @@ -35,6 +35,7 @@ properties:
> > >        - description: Rockchip designed configuration registers
> > >        - description: Config registers
> > >        - description: iATU registers
> > > +      - description: eDMA registers
> > 
> > Same here, is this just for one of the two supported models, or for
> > both?
> 
> For the 3 controllers found in rk3568, this range exists for all
> (all of the controllers were synthesized with the eDMA controller).
> 
> For the 5 controllers found in rk3588, this range exists for only one of them
> (only one of the controllers was synthesized with the eDMA controller).
> 
> 
> > >    interrupt-names:
> > > +    minItems: 5
> > >      items:
> > >        - const: sys
> > >        - const: pmc
> > >        - const: msg
> > >        - const: legacy
> > >        - const: err
> > > +      - const: dma0
> > > +      - const: dma1
> > > +      - const: dma2
> > > +      - const: dma3
> 
> While all the PCIe controllers on the rk3568 have the embedded DMA controller
> as part of the PCIe controller, they don't have separate IRQs for the eDMA.
> (They will need to use the combined "sys" irq, so the driver will need to read
> an additional register to see that it was an eDMA irq.)
> 
> For the rk3588, only one of the 5 PCIe controllers have the eDMA, and that
> controller also has dedicated IRQs for the eDMA.
> (It should also be able to use the combined "sys" irq, but that would be less
> efficient, and AFAICT, the driver currently only works with dedicated IRQs.)

We could go with Sebastian's suggestion to define a 1MB range for "atu", see:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/pci/snps%2Cdw-pcie.yaml#L76-L85

Which would allow the driver to probe if the eDMA is there or not
(even if this is strictly bigger than the real ATU_CAP size, the size is still
within the PCIe core's register map).

That would solve the problem that some pcie controllers, with the exact same
compatible, has a "dma" range while others do not.
(All controllers would have a 1MB atu range, and none of them would have the
dma range specified.)

However, we would still have the problem that for the exact same compatible,
some controllers have eDMA irqs specified in interrupts, and some do not...

But perhaps having mandatory atu range (and no dma range) + optional dma irqs
is better than mandatory atu range + optional dma range + optional dma irqs?
(At least from a DT schema maintainability PoV.)


Kind regards,
Niklas