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

[Serge Semin]

Hi Niklas

On Wed, Oct 25, 2023 at 08:55:33PM +0000, Niklas Cassel wrote:
> 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 at 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 at 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

Are you sure the eDMA regs aren't just a part of the same space as
iATU and available within the default offset DEFAULT_DBI_DMA_OFFSET?
If it is then drop separate eDMA reg, having just 'atu' will be enough.

> > > 
> > > 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.)

If it's some vendor-specific register, then the best solution would be
to create a custom IRQ domain with 'sys' IRQ being a parental IRQ and
with the "dma*" IRQs being the child IRQs. Then you can re-define the
dw_pcie_edma_ops.irq_vector method (and edma.nr_irqs field), which
would return the DMA IRQs from the new sub-domain.

If it's just all the IRQs combined in a single "sys" IRQ line with no
custom way to distinguish one from another, then you can re-define the
dw_pcie_edma_ops.irq_vector method with a method returning the 'sys'
IRQ vector (don't forget to set the edma.nr_irqs field). eDMA IRQ
handler will check whether the IRQ was originated by the eDMA
controller. 

> > 

> > 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.)

This is a standard way of the eDMA IRQs definition. So supposedly there won't
be problem with rk3588

> 
> 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

Right, That's what I asked in my very first message.

> 
> 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...

You can define the normal eDMA IRQs as optional for all Rockchip
controllers (or for rk3588 only). They will be auto-detected by the
DW PCIe core driver as expected if the standard eDMA IRQs are
available. But for the rk3568 you'll need to redefine the
dw_edma_chip.ops operation to have a custom irq_vector() callback as I
described above.

> 
> 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.)

As I mentioned in the first message, if eDMA CSRs are available within
the standard offset PCIE_DMA_UNROLL_BASE with respect to the iATU CSRs
base, then that is a single CSRs space (selected by the DBI_CS2=1 and
CDM_SB=1 internal IP-core signals) and just 'atu' reg is supposed to
be specified.

-Serge(y)

> 
> 
> Kind regards,
> Niklas