| Message ID | 20240619112406.106223-1-matthias.schiffer@ew.tq-group.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dt-bindings: soc: ti: pruss: allow ethernet controller in ICSSG node | expand |
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? > > - It makes sense semantically - the Ethernet controller is running on > the ICSSG/PRUSS > - Disabling or deleting the ICSSG node implicitly removes the Ethernet > controller node when it is a child node. This can be relevant on SoCs > like the AM64x which come in variants with and without ICSSG; e.g., on > the TQMa64xxL the ICSSG node will be disabled on variants without as a > bootloader fixup. > On Linux, this avoids leaving the Ethernet controller in deferred > state forever while waiting for the ICSSG to become available > (resulting in a warning on newer kernels) > > The node name "ethernet" is chosen as it nicely matches the regular > "ethernet@<reg>" format of many Ethernet controller nodes, and is also > what the prueth binding example (/schemas/net/ti,icssg-prueth.yaml) uses. > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > --- > Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml > index c402cb2928e89..89dfcf5ce8434 100644 > --- a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml > +++ b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml > @@ -92,6 +92,13 @@ properties: > description: | > This property is as per sci-pm-domain.txt. > > + ethernet: > + description: | Do not need '|' unless you need to preserve formatting. > + ICSSG PRUSS Ethernet. Configuration for an Ethernet controller running > + on the PRU-ICSS. > + $ref: /schemas/net/ti,icssg-prueth.yaml# > + type: object > + > patternProperties: > > memories@[a-f0-9]+$: You are mixing MMIO and non-MMIO nodes. That's odd or even sloppy design. It immediately raises questions about your bindings. Best regards, Krzysztof
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. 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). Best regards, Matthias > > > > > - It makes sense semantically - the Ethernet controller is running on > > the ICSSG/PRUSS > > - Disabling or deleting the ICSSG node implicitly removes the Ethernet > > controller node when it is a child node. This can be relevant on SoCs > > like the AM64x which come in variants with and without ICSSG; e.g., on > > the TQMa64xxL the ICSSG node will be disabled on variants without as a > > bootloader fixup. > > On Linux, this avoids leaving the Ethernet controller in deferred > > state forever while waiting for the ICSSG to become available > > (resulting in a warning on newer kernels) > > > > The node name "ethernet" is chosen as it nicely matches the regular > > "ethernet@<reg>" format of many Ethernet controller nodes, and is also > > what the prueth binding example (/schemas/net/ti,icssg-prueth.yaml) uses. > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > > --- > > Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml > > index c402cb2928e89..89dfcf5ce8434 100644 > > --- a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml > > +++ b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml > > @@ -92,6 +92,13 @@ properties: > > description: | > > This property is as per sci-pm-domain.txt. > > > > + ethernet: > > + description: | > > Do not need '|' unless you need to preserve formatting. > > > + ICSSG PRUSS Ethernet. Configuration for an Ethernet controller running > > + on the PRU-ICSS. > > + $ref: /schemas/net/ti,icssg-prueth.yaml# > > + type: object > > + > > patternProperties: > > > > memories@[a-f0-9]+$: > > You are mixing MMIO and non-MMIO nodes. That's odd or even sloppy > design. It immediately raises questions about your bindings. > > Best regards, > Krzysztof >
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. Best regards, Krzysztof
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). 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. 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. Best regards, Matthias > > > Best regards, > Krzysztof >
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
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 >
diff --git a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml index c402cb2928e89..89dfcf5ce8434 100644 --- a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml +++ b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml @@ -92,6 +92,13 @@ properties: description: | This property is as per sci-pm-domain.txt. + ethernet: + description: | + ICSSG PRUSS Ethernet. Configuration for an Ethernet controller running + on the PRU-ICSS. + $ref: /schemas/net/ti,icssg-prueth.yaml# + type: object + patternProperties: memories@[a-f0-9]+$:
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: - It makes sense semantically - the Ethernet controller is running on the ICSSG/PRUSS - Disabling or deleting the ICSSG node implicitly removes the Ethernet controller node when it is a child node. This can be relevant on SoCs like the AM64x which come in variants with and without ICSSG; e.g., on the TQMa64xxL the ICSSG node will be disabled on variants without as a bootloader fixup. On Linux, this avoids leaving the Ethernet controller in deferred state forever while waiting for the ICSSG to become available (resulting in a warning on newer kernels) The node name "ethernet" is chosen as it nicely matches the regular "ethernet@<reg>" format of many Ethernet controller nodes, and is also what the prueth binding example (/schemas/net/ti,icssg-prueth.yaml) uses. Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> --- Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml | 7 +++++++ 1 file changed, 7 insertions(+)