[PATCH] dt-bindings: pci: brcmstb: Add rp1-nexus node to fix DTC warning
Andrea della Porta
andrea.porta at suse.com
Fri Dec 12 02:52:44 PST 2025
Hi Herve, Rob,
On 15:00 Wed 10 Dec , Herve Codina wrote:
> Hi Andrea,
>
> On Tue, 9 Dec 2025 19:27:03 +0100
> Andrea della Porta <andrea.porta at suse.com> wrote:
>
> > [+cc Herve]
> >
> > On 18:58 Tue 09 Dec , Andrea della Porta wrote:
> > > Hi Rob,
> > >
> > > On 11:09 Thu 04 Dec , Rob Herring wrote:
> > > > On Mon, Sep 1, 2025 at 3:48 AM Andrea della Porta <andrea.porta at suse.com> wrote:
> > > > >
> > > > > Hi Krzysztof,
> > > > >
> > > > > On 08:50 Fri 22 Aug , Krzysztof Kozlowski wrote:
> > > > > > On 21/08/2025 17:22, Andrea della Porta wrote:
> > > > > > > Hi Krzysztof,
> > > > > > >
> > > > > > > On 10:55 Tue 12 Aug , Krzysztof Kozlowski wrote:
> > > > > > >> On 12/08/2025 10:50, Andrea della Porta wrote:
> > > > > > >>> The devicetree compiler is complaining as follows:
> > > > > > >>>
> > > > > > >>> arch/arm64/boot/dts/broadcom/rp1-nexus.dtsi:3.11-14.3: Warning (unit_address_vs_reg): /axi/pcie at 1000120000/rp1_nexus: node has a reg or ranges property, but no unit name
> > > > > > >>> /home/andrea/linux-torvalds/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: pcie at 1000120000: Unevaluated properties are not allowed ('rp1_nexus' was unexpected)
> > > > > > >>
> > > > > > >> Please trim the paths.
> > > > > > >
> > > > > > > Ack.
> > > > > > >
> > > > > > >>
> > > > > > >>>
> > > > > > >>> Add the optional node that fix this to the DT binding.
> > > > > > >>>
> > > > > > >>> Reported-by: kernel test robot <lkp at intel.com>
> > > > > > >>> Closes: https://lore.kernel.org/oe-kbuild-all/202506041952.baJDYBT4-lkp@intel.com/
> > > > > > >>> Signed-off-by: Andrea della Porta <andrea.porta at suse.com>
> > > > > > >>> ---
> > > > > > >>> Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 9 +++++++++
> > > > > > >>> 1 file changed, 9 insertions(+)
> > > > > > >>>
> > > > > > >>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > > > >>> index 812ef5957cfc..7d8ba920b652 100644
> > > > > > >>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > > > >>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > > > >>> @@ -126,6 +126,15 @@ required:
> > > > > > >>> allOf:
> > > > > > >>> - $ref: /schemas/pci/pci-host-bridge.yaml#
> > > > > > >>> - $ref: /schemas/interrupt-controller/msi-controller.yaml#
> > > > > > >>> + - if:
> > > > > > >>> + properties:
> > > > > > >>> + compatible:
> > > > > > >>> + contains:
> > > > > > >>> + const: brcm,bcm2712-pcie
> > > > > > >>> + then:
> > > > > > >>> + properties:
> > > > > > >>> + rp1_nexus:
> > > > > > >>
> > > > > > >> No, you cannot document post-factum... This does not follow DTS coding
> > > > > > >> style.
> > > > > > >
> > > > > > > I think I didn't catch what you mean here: would that mean that
> > > > > > > we cannot resolve that warning since we cannot add anything to the
> > > > > > > binding?
> > > > > >
> > > > > > I meant, you cannot use a warning from the code you recently introduced
> > > > > > as a reason to use incorrect style.
> > > > > >
> > > > > > Fixing warning is of course fine and correct, but for the code recently
> > > > > > introduced and which bypassed ABI review it is basically like new review
> > > > > > of new ABI.
> > > > > >
> > > > > > This needs standard review practice, so you need to document WHY you
> > > > > > need such node. Warning is not the reason here why you are doing. If
> > > > > > this was part of original patchset, like it should have been, you would
> > > > > > not use some imaginary warning as reason, right?
> > > > > >
> > > > > > So provide reason why you need here this dedicated child, what is that
> > > > > > child representing.
> > > > >
> > > > > Ack.
> > > > >
> > > > > >
> > > > > > Otherwise I can suggest: drop the child and DTSO, this also solves the
> > > > > > warning...
> > > > >
> > > > > This would not fix the issue: it's the non overlay that needs the specific
> > > > > node. But I got the point, and we have a solution for that (see below).
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Regarding rp1_nexus, you're right I guess it should be
> > > > > > > rp1-nexus as per DTS coding style.
> > > > > > >
> > > > > > >>
> > > > > > >> Also:
> > > > > > >>
> > > > > > >> Node names should be generic. See also an explanation and list of
> > > > > > >> examples (not exhaustive) in DT specification:
> > > > > > >> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> > > > > > >
> > > > > > > In this case it could be difficult: we need to search for a DT node
> > > > > >
> > > > > > Search like in driver? That's wrong, you should be searching by compatible.
> > > > >
> > > > > Thanks for the hint. Searching by compatble is the solution.
> > > >
> > > > No, it is not.
> > >
> > > This is partly true, indeed. On one side there's the need to avoid a
> > > specific node name ('rp1_nexus'), so the only other unique identifier would
> > > be the compatible string ('pci1de4,1' in this case, which identifies that specific
> > > device). Unfortunately, the same compatible string is also assigned to the pci
> > > endpoint node filled automatically by enabling CONFIG_PCI_DYNAMIC_OF_NODES.
> > > We would end up with two nodes with the same compatible, which is not unique
> > > anymore.
> > > This applies only when using 'full' dtb (bcm2712-rpi-5-b.dtb) *and* you enable
> > > CONFIG_PCI_DYNAMIC_OF_NODES, the latter being not necessary since the overlay dtb
> > > (...-ovl-rp1.dtb) is not in use here. To overcome this problem, the solutions
> > > I can think of are the following:
> > >
> > > 1- Just disable CONFIG_PCI_DYNAMIC_OF_NODES should work, but only when using the
> > > full dtb version. However, if the user enable that option for debug or to use
> > > the overlay dtb version, he better be sure not to use teh full dtb or it won't
> > > work.
> > > This solution seems really weak.
> > >
> > > 2- Add another compatible string other than 'pci1de4,1', so it will be really
> > > unique.
> > >
> > > >
> > > > > >
> > > > > > > starting from the DT root and using generic names like pci at 0,0 or
> > > > > > > dev at 0,0 could possibly led to conflicts with other peripherals.
> > > > > > > That's why I chose a specific name.
> > > > > >
> > > > > > Dunno, depends what can be there, but you do not get a specific
> > > > > > (non-generic) device node name for a generic PCI device or endpoint.
> > > > >
> > > > > I would use 'port' instead of rp1-nexus. Would it work for you?
> > > >
> > > > Do you still plan to fix this? This is broken far worse than just the node name.
> > >
> > > Yes, if we want to get rid of that nasty warning and comply with DT guidelines,
> > > I think I really need to fix that.
> > >
> > > >
> > > > The 'rp1_nexus' node is applied to the PCI host bridge. That's wrong
> > > > unless this is PCI rather than PCIe. There's the root port device in
> > > > between.
> > > >
> > > > The clue that things are wrong are start in the driver here:
> > > >
> > > > rp1_node = of_find_node_by_name(NULL, "rp1_nexus");
> > > > if (!rp1_node) {
> > > > rp1_node = dev_of_node(dev);
> > > > skip_ovl = false;
> > > > }
>
> I don't fully understand what you want to do.
>
> This piece of code is present in the drivers/misc/rp1/rp1_pci.c driver.
> This driver is used when a specific PCI device is available on the system.
> This device is PCI_DEVICE(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RPI_RP1_C0)
>
> In order to work, the driver needs either CONFIG_PCI_DYNAMIC_OF_NODES or
> having the full PCI hierarchy described in the DT leading to this device.
> Indeed, it needs a DT node related to the PCI device.
>
> I don't understand why rp1_nexus should be described at the PCI controller
> root level.
>
> Only PCI devices (or bridges as they are particular devices) at this level
> as decribed in the pci-bus-common.yaml binding [1].
Ack. As you and Rob pointed out, I will amend the full DT hierarchy with the
PCI nodes in the way as they are dynamically created by CONFIG_PCI_DYNAMIC_OF_NODES.
>
> If the purpose of this 'rp1_nexus' node is to only avoid the application of
> the overlay, even if I don't understand why, you should search for something
> added by the overlay and not for the 'rp1_nexus' node in the whole DT.
I cannot search for something added by the overlay if I need to choose whether
to apply the overlay or not in teh first place (iIOW the overlay is not yet loaded).
This is not of a concern however, since I'm planning to get rid of the overlay
for reasons already explained here [1]. This will not only solve all of this
problems but it will also address Rob's concern of not relying on
CONFIG_PCI_DYNAMIC_OF_NODES.
>
> Instead of:
> rp1_node = of_find_node_by_name(NULL, "rp1_nexus");
>
> You should search for a sub-node added by the overlay. Something like the
> following for instance:
> --- 8< ---
> int rp1_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
> struct device_node *np;
>
> np = dev_of_node(&pdev->dev);
>
> ...
> if ( np && of_find_node_by_name(np, "pci-ep-bus at 1");
> ...
> }
> --- 8< ---
>
> If you really want to be sure that the "pci-ep-bus at 1" comes from an overlay
> you can check for the OF_OVERLAY flag.
> --- 8< ---
> node = of_find_node_by_name(np, "pci-ep-bus at 1");
> if (node && of_node_check_flag(node, OF_OVERLAY))
> --- 8< ---
>
> Also your overlay has to be applied on the DT node related to your PCI
> device and not the rp1_nexus you search for in the whole DT. In other
> word, in your driver, the following is not correct:
>
> err = of_overlay_fdt_apply(dtbo_start, dtbo_size, &rp1->ovcs_id,
> rp1_node);
>
> It should be:
>
> err = of_overlay_fdt_apply(dtbo_start, dtbo_size, &rp1->ovcs_id,
> dev_of_node(&pdev->dev));
>
> In your overlay, your have
> __overlay__ {
> compatible = "pci1de4,1";
> #address-cells = <3>;
> #size-cells = <2>;
> interrupt-controller;
> #interrupt-cells = <2>;
>
> #include "arm64/broadcom/rp1-common.dtsi"
> };
>
> Only sub-nodes should be present (#address-cells = <3> and #size-cells = <2>)
> can be there just to make DTC happy.
>
> It is worth noting that any properties other than #address-cells and
> #size-cells lead to memleaks. And so avoid them.
>
> Memleaks are reported at runtime when the overlay is applied with this
> kind of logs:
> OF: overlay: WARNING: memory leak will occur if overlay removed, property: ...
>
> I am pretty sure that you have those kind of traces when you apply your
> overlay.
Indeed.
Many thanks,
Andrea
[1] - https://lore.kernel.org/all/aTvtr5v4DjuqctdY@apocalypse/
>
> compatible, interrupt-controller and #interrupt-cells should not be added by
> the overlay. Either they are added by when PCI nodes are created by
> CONFIG_PCI_DYNAMIC_OF_NODES either they are already available in the base
> DT if the PCI device is described in the base DT.
>
> [1] https://github.com/devicetree-org/dt-schema/blob/aa859412ce8e38c63bb13fa55c5e1c6ba66a8e3b/dtschema/schemas/pci/pci-bus-common.yaml#L226
>
> Best regards,
> Hervé
More information about the linux-arm-kernel
mailing list