Date: Wed, 26 Jun 2024 09:33:27 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Matthias Schiffer <matthias.schiffer@...tq-group.com>
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 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.

> 
> 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

Powered by blists - more mailing lists