Re: [PATCH net-next 09/13] dt-bindings: net: airoha: Add airoha,npu phandle property

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2824 bytes --]

On Wed, Feb 05, 2025 at 09:52:14PM +0100, Lorenzo Bianconi wrote:
> > On Wed, Feb 05, 2025 at 08:33:13PM +0100, Lorenzo Bianconi wrote:
> > > > On Wed, Feb 05, 2025 at 07:21:28PM +0100, Lorenzo Bianconi wrote:
> > > > > Introduce the airoha,npu property for the npu syscon node available on
> > > > > EN7581 SoC. The airoha Network Processor Unit (NPU) is used to offload
> > > > > network traffic forwarded between Packet Switch Engine (PSE) ports.
> > > > > 
> > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml | 8 ++++++++
> > > > >  1 file changed, 8 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > > > > index c578637c5826db4bf470a4d01ac6f3133976ae1a..6388afff64e990a20230b0c4e58534aa61f984da 100644
> > > > > --- a/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > > > > +++ b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > > > > @@ -63,6 +63,12 @@ properties:
> > > > >    "#size-cells":
> > > > >      const: 0
> > > > >  
> > > > > +  airoha,npu:
> > > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > > +    description:
> > > > > +      Phandle to the syscon node used to configure the NPU module
> > > > > +      used for traffic offloading.
> > > > 
> > > > Why do you need a phandle for this, instead of a lookup by compatible?
> > > > Do you have multiple instances of this ethernet controller on the
> > > > device, that each need to look up a different npu?
> > > 
> > > actually not, but looking up via the compatible string has been naked by
> > > Krzysztof on a different series [0].
> > 
> > Hmm, I disagree with adding phandles that are not needed. I don't agree
> > that there's no reuse, if you can treat the phandle identically on a new
> > device, in all likelihood, that node should have a fallback to the
> > existing one.
> 
> honestly I do not have a strong opinion about it, I am fine both ways.
> 
> > 
> > That said, the bigger problem in what you link is the ABI break caused by
> > requiring the presence of a new node. I'd NAK that patch too.
> 
> there is no PCIe support in dts for this device upstream yet, so I guess there
> is no ABI break, right?

The binding and driver are upstream and therefore there could be users.
Whether or not the dts is upstream isn't what determines ABI break or not!
If the thing worked before adding the syscon in the PCIe patch you'd linked,
the syscon should remain optional - unless there really are no users
that will be impacted. If it didn't work without the syscon, then I
wonder why it was upstreamed like that at all.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]