Re: [PATCH 04/12] dt-bindings: rockchip: Add DesignWare based PCIe Endpoint controller

[Niklas Cassel <cassel@kernel.org>]

On Thu, Apr 25, 2024 at 11:08:09AM -0500, Rob Herring wrote:
> On Wed, Apr 24, 2024 at 05:16:22PM +0200, Niklas Cassel wrote:
> > Document DT bindings for PCIe Endpoint controller found in Rockchip SoCs.
> > 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > ---
> >  .../bindings/pci/rockchip-dw-pcie-ep.yaml          | 192 +++++++++++++++++++++
> >  1 file changed, 192 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-ep.yaml
> > new file mode 100644
> > index 000000000000..57a6c542058f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-ep.yaml
> > @@ -0,0 +1,192 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie-ep.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: DesignWare based PCIe Endpoint controller on Rockchip SoCs
> > +
> > +maintainers:
> > +  - Niklas Cassel <cassel@kernel.org>
> > +
> > +description: |+
> > +  RK3588 SoC PCIe Endpoint controller is based on the Synopsys DesignWare
> > +  PCIe IP and thus inherits all the common properties defined in
> > +  snps,dw-pcie-ep.yaml.
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/snps,dw-pcie-ep.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: rockchip,rk3588-pcie-ep
> 
> 3568 doesn't support endpoint mode? It would be good to keep the 
> bindings aligned.

It does.
However, it does not have the dedicated IRQ lines for the eDMA interrupts.
I will add rk3568 to the DT binding and to the driver.
If someone wants eDMA functional for rk3568, there is further code needed,
but EP mode without eDMA should work on rk3568 as is.


> > +  phys:
> > +    maxItems: 1
> > +
> > +  phy-names:
> > +    const: pcie-phy
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 2
> > +
> > +  reset-names:
> > +    items:
> > +      - const: pwr
> > +      - const: pipe
> 
> Most of this is all duplicated from rockchip-dw-pcie.yaml. Pull out the 
> common bits to a separate schema file and reference it from the RC and 
> endpoint schemas.

Ok, will fix in V2.


> You'll need to add to interrupts/interrupt-names in the common schema 
> and then restrict the number of items here and in the RC schema.

Remember that eDMA can be used also in RC mode.
Even if the RC binding doesn't allow it right now, these interrupts could
be optional also for RC mode, in case someone actually wants to use them
in the future.


> > +
> > +  vpcie3v3-supply: true
> 
> This doesn't make sense for endpoint mode. At least in the sense this 
> is supposed to be a standard slot voltage driven from the host side.

I tried not supplying the regulator for the EP side on my rock5b
(rk3588 based) platform.
The driver (in EP mode) probes correctly, but does not work without this,
regardless of how I try. Boot EP first, boot RC first.

Looking at the rock5b schematic:
https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock_5b_v1423_sch.pdf
Page 7, specifically VCC3V3_PCIE30.
It does seem to only support sourcing VIN from a regulator on the local
board (VCC5V0_SYS).

(Looking at a vendor using this SoC in a board that supports EP mode
(Mixtile Blade 3), they do supply the regulator also for the EP-mode
DT node.)

I will drop the "vpcie3v3-supply" from the EP binding and keep it
only in the RC binding. (As perhaps some other rk3588 based board can
actually source the 3.3v from the PCIe slot in EP mode.)

I will keep it in the rock5b (a rk3588 based board) DT overlay,
as it is obviously needed for rock5b.


> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - interrupt-names
> > +  - num-lanes
> > +  - phys
> > +  - phy-names
> > +  - power-domains
> > +  - resets
> > +  - reset-names
> 
> A bunch or all? of these can be in the common schema too.

Ok, will fix in V2.


Kind regards,
Niklas