Hi,
On Fri, Oct 20, 2023 at 11:58:40PM +0000, Niklas Cassel wrote:
> On Fri, Oct 20, 2023 at 11:23:57PM +0200, Sebastian Reichel wrote:
> > On Fri, Oct 20, 2023 at 02:52:20PM +0200, Niklas Cassel wrote:
> > > From the snps,dw-pcie.yaml devicetree binding:
> > > "At least DBI reg-space and peripheral devices CFG-space outbound window
> > > are required for the normal controller work. iATU memory IO region is
> > > also required if the space is unrolled (IP-core version >= 4.80a)."
> > >
> > > All the PCIe controllers in rk3588 are using the iATU unroll feature,
> > > and thus have to supply the atu property in the device tree node.
> > >
> > > The size of the iATU region equals to:
> > > MAX(num inbound ATU regions, num outbound ATU regions) * 0x200.
> > >
> > > Where for each 0x200 region, the registers controlling the
> > > IATU_REGION_OUTBOUND starts at offset 0x0, and the registers controlling
> > > IATU_REGION_INBOUND starts at offset 0x100.
> > >
> > > pcie3x4 and pcie3x2 have 16 ATU inbound regions, 16 ATU outbound regions,
> > > so they have size: max(16, 16) * 0x200 = 0x2000
> > >
> > > pcie2x1l0, pcie2x1l1, and pcie2x1l2 have 8 ATU inbound regions, 8 ATU
> > > outbound regions, so they have size: max(8, 8) * 0x200 = 0x1000
> > >
> > > On the rk3588 based rock-5b board:
> > > Before this patch, dw_pcie_iatu_detect() fails to detect all iATUs:
> > > rockchip-dw-pcie a40000000.pcie: iATU: unroll T, 8 ob, 8 ib, align 64K, limit 8G
> > > rockchip-dw-pcie a41000000.pcie: iATU: unroll T, 8 ob, 8 ib, align 64K, limit 8G
> > > rockchip-dw-pcie a40800000.pcie: iATU: unroll T, 8 ob, 8 ib, align 64K, limit 8G
> > >
> > > After this patch, dw_pcie_iatu_detect() succeeds to detect all iATUs:
> > > rockchip-dw-pcie a40000000.pcie: iATU: unroll T, 16 ob, 16 ib, align 64K, limit 8G
> > > rockchip-dw-pcie a41000000.pcie: iATU: unroll T, 8 ob, 8 ib, align 64K, limit 8G
> > > rockchip-dw-pcie a40800000.pcie: iATU: unroll T, 8 ob, 8 ib, align 64K, limit 8G
> > >
> > > Fixes: 8d81b77f4c49 ("arm64: dts: rockchip: add rk3588 PCIe2 support")
> > > Fixes: 0acf4fa7f187 ("arm64: dts: rockchip: add PCIe3 support for rk3588")
> > > Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> > > ---
> >
> > Thanks for your patch. This looks sensible, but I have two comments:
> >
> > 1. You need to update the Rockchip DT binding. It currently enforces that regnames
> > must be "dbi", "apb", "config". Thus 'make CHECK_DTBS=y rockchip/rk3588-rock-5b.dtb'
> > will introduce new errors after this patch is applied.
>
> In the rockchip-dw-pcie.yaml:
>
> RK3568 SoC PCIe host controller is based on the Synopsys DesignWare
> PCIe IP and thus inherits all the common properties defined in
> snps,dw-pcie.yaml.
>
> allOf:
> - $ref: /schemas/pci/snps,dw-pcie.yaml#
>
> So it obviously references the snps,dw-pcie.yaml using allOf. snps,dw-pcie.yaml
> is the schema that has the atu property defined. If the validation tooling does
> not support inherited properties, that is a bit disappointing, but not something
> that I think should stop this patch, which solves a real problem, from being
> accepted.
The validation tooling does support inherited properties. The
Rockchip binding just adds further restrictions. Those restrictions
need to be updated.
As stated in Documentation/process/maintainer-soc-clean-dts.rst this
needs to happen for your change to be accepted.
> > 2. You calculated the exact ATU size and used that, but the binding specifies,
> > "iATU/eDMA registers common for all device functions. [...] Note iATU is normally
> > mapped to the 0x0 address of this region, while eDMA is available at 0x80000 base
> > address.". So the size should be big enough to include the DMA section. So I would
> > expect it to be 0x100000 for all controllers.
>
> The device tree schema also allows you to specify the eDMA region using a separate
> "dma" property.
>
> This is what I chose to do, see my patch in:
> https://lore.kernel.org/linux-devicetree/20231020224412.3722784-1-nks@xxxxxxxxxxx/T/#u
>
> [long explanation]
That only arrived in my mailbox after I reviewed this patch :)
FWIW that way is also fine with me, but also needs updated DT
binding. I suggest to send everything as a series in v2. These
patches basically belong together, since the memory section for
DMA is removed in this patch.
Greetings,
-- Sebastian
Attachment:
signature.asc
Description: PGP signature
