[PATCH] dt-bindings: pci: brcmstb: Add rp1-nexus node to fix DTC warning
Herve Codina
herve.codina at bootlin.com
Wed Dec 10 06:00:08 PST 2025
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].
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.
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.
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