Re: [PATCH] dt-bindings: pci: brcmstb: Add rp1-nexus node to fix DTC warning

[Andrea della Porta <andrea.porta@suse.com>]

[+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@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@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@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@intel.com>
> > > > >>> Closes: https://lore.kernel.org/oe-kbuild-all/202506041952.baJDYBT4-lkp@intel.com/
> > > > >>> Signed-off-by: Andrea della Porta <andrea.porta@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@0,0 or
> > > > > dev@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;
> > }
> > 
> > You should not need to do this nor care what the node name is. The PCI
> > core should have populated pdev->dev.of_node for you. If not, your DT
> > is wrong. Turn on CONFIG_PCI_DYNAMIC_OF_NODES and look at the
> > resulting PCI nodes. They should also match what the hierarchy looks
> > like with lspci. I don't recommend you rely on
> > CONFIG_PCI_DYNAMIC_OF_NODES, but statically populate the nodes in the
> > DT. First, CONFIG_PCI_DYNAMIC_OF_NODES is an under development thing
> > and I hope to get rid of the config option. Second, your case is
> > static (i.e. not a PCIe card in a slot) so there is no issue
> > hardcoding the DT.

Are you planning to get rid of the CONFIG_PCI_DYNAMIC_OF_NODES features?
There could be other drivers relying on that besides RP1 in overlay mode,
e.g. LAN966x for instance.

Thanks,
Andrea

> 
> The 'full' dtb (bcm2712-rpi-5-b.dtb) is indeed statically populated.
> CONFIG_PCI_DYNAMIC_OF_NODES is needed only if you use the overlay approach
> (bcm2712-rpi-5-b-ovl-rp1.dtb) and in that case the node will be
> added to the correct device node at runtime, and there won't be any node
> labeled as rp1_nexus.
> That conditional just check for teh presence of the rp1_nexus node to
> choose if the overlay should be loaded at runtime (if rp1_nexus is present,
> then we are in the static case so no overlay need to be loaded).
> 
> > 
> > As far as the node name, I don't care so much as long as the driver
> > doesn't care and you don't use '_' or 'pcie?' (that's for PCI
> > bridges).
> > 
> > And why do we have drivers/misc/rp1/rp1-pci.dtso and a .dtso in
> > arch/arm64? There should not be both.
> 
> drivers/misc/rp1/rp1-pci.dtso is just a wrapper over rp1-common.dtsi,
> which is to be compiled in as binary blob in the driver and loaded at
> runtime.
> rp1.dtbo is optional, it's there just for completeness in case anyone want
> to load teh overlay from the fw, as explained here:
> https://lore.kernel.org/all/ab9ab3536baf5fdf6016f2a01044f00034189291.1742418429.git.andrea.porta@suse.com/
> If it causes confusion, we can probably get rid of it with no penalty.
> 
> Many thanks,
> Andrea
> 
> > 
> > Rob