Message-ID: <958090771f1b301f00b96d55aac312704866f1bb.camel@ew.tq-group.com>
Date: Wed, 26 Jun 2024 10:18:08 +0200
From: Matthias Schiffer <matthias.schiffer@...tq-group.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
 linux-arm-kernel@...ts.infradead.org, linux@...tq-group.com, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Nishanth Menon <nm@...com>, Vignesh Raghavendra
 <vigneshr@...com>, Tero Kristo <kristo@...nel.org>, Suman Anna
 <s-anna@...com>
Subject: Re: [PATCH 1/2] dt-bindings: soc: ti: pruss: allow ethernet
 controller in ICSSG node

On Wed, 2024-06-26 at 09:33 +0200, Krzysztof Kozlowski wrote:
> 
> On 20/06/2024 10:48, Matthias Schiffer wrote:
> > On Thu, 2024-06-20 at 10:29 +0200, Krzysztof Kozlowski wrote:
> > > 
> > > 
> > > On 20/06/2024 10:26, Matthias Schiffer wrote:
> > > > On Thu, 2024-06-20 at 09:24 +0200, Krzysztof Kozlowski wrote:
> > > > > On 19/06/2024 13:24, Matthias Schiffer wrote:
> > > > > > While the current Device Trees for TI EVMs configure the PRUSS Ethernet
> > > > > > controller as a toplevel node with names like "icssg1-eth", allowing to
> > > > > > make it a subnode of the ICSSG has a number of advantages:
> > > > > 
> > > > > What is ICSSG? The sram or ti,prus from the ethernet schema?
> > > > 
> > > > ICSSG (Industrial Communication Subsystem (Group?)) is the main device described by the
> > > > ti,pruss.yaml binding (ICSS and PRUSS are different variants of similar IP cores); it is the
> > > > container for the individual PRU, TXPRU and RTU cores which are referenced by the ti,prus
> > > > node of the Ethernet schema.
> > > > 
> > > > The entirety of PRU, TXPRU and RTU cores of one ICSSG, each with its own firmware, forms one
> > > > Ethernet controller, which is not quite a hardware device, but also not a fully virtual software
> > > > device.
> > > 
> > > So it is not really child of ICSSG.
> > > 
> > > > 
> > > > The Ethernet controller only exists through the various ICSS subcores, so it doesn't have an MMIO
> > > > address of its own. As described, the existing Device Trees define it as a toplevel non-MMIO node;
> > > > we propose to allow it as a non-MMIO child node of the ICSSG container instead.
> > > > 
> > > > If you consider moving the ethernet node into the ICSSG node a bad approach, we will drop this patch
> > > > and try to find a different solution to our issue (the Ethernet device staying in deferred state
> > > > forever when the ICSSG node is disabled on Linux).
> > > 
> > > Just disable the ethernet. That's the expected behavior, I don't get
> > > what is the problem here.
> > 
> > If the disabling happens as a fixup in the bootloader, it needs to know the name of the Ethernet
> > controller node (or iterate through the DTB to find references to the disabled ICSSG node).
> 
> Which is already solved for several such cases, including ethernet
> devices? Aliases?
> 
> > 
> > The name is currently not used for anything, and not specified in the binding doc; the example uses
> > "ethernet", which is too unspecific, as there can be multiple ICSSG/PRUs, with each running a
> > separate Ethernet controller.
> 
> Use existing solutions - aliases.

Understood.

I'm not entirely happy that the bootloader needs to know that it is an Ethernet controller that is
provided by the ICSSG, and there isn't a simple way to say "whatever kind of device that Linux's DTB
loads into the ICSSG should be disabled".

But I guess for most boards there is only a single kind of ICSSG firmware that is used anyways. So
I'm going with the solution you propose for now.

Best regards,
Matthias


> 
> > 
> > Existing Device trees use "icssgX-eth" for an Ethernet controller running on the ICSSG with label
> > "&icssgX", but labels are a source concept and don't exist in the compiled DTB by default.
> > 
> > I do have an idea for an alternative approach that does not need changes to the DT bindings: The PRU
> > Ethernet driver could detect that the referenced ti,prus are disabled and not just waiting to be
> > probed and then fail with ENODEV instead of EPROBE_DEFER.
> 
> Sorry, but re-shuffling nodes into incorrect hardware description is not
> the workaround for your problem.
> 
> Best regards,
> Krzysztof
> 

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Powered by blists - more mailing lists