From: Zhi Li <lizhi2@eswincomputing.com>
The second Ethernet controller (eth1) on the EIC7700 SoC may experience
RX data sampling issues at high speed due to EIC7700-specific receive
clock to data skew at the MAC input.
On the EIC7700 SoC, the second Ethernet controller (eth1) requires
inversion of the internal RGMII receive clock in order to meet RX data
sampling timing at high speed.
Describe this SoC-specific difference by introducing a distinct compatible
string for MAC instances that require internal clock inversion, allowing the
driver to select the appropriate configuration without relying on per-board
vendor-specific properties.
The rx-internal-delay-ps and tx-internal-delay-ps properties now use
minimum and maximum constraints to reflect the actual hardware delay
range (0-2540 ps) applied in 20 ps steps. This relaxes the binding
validation compared to the previous enum-based definition and avoids
regressions for existing DTBs while keeping the same hardware limits.
Treat the RX/TX internal delay properties as optional, board-specific
tuning knobs and remove them from the example to avoid encouraging
their use.
In addition, the binding now includes additional background information
about the HSP CSR registers accessed by the MAC. The TXD and RXD delay
control registers are included so the driver can explicitly clear any
residual configuration left by the bootloader. Background reference for
the High-Speed Subsystem and HSP CSR block is available in Chapter 10
("High-Speed Interface") of the EIC7700X SoC Technical Reference Manual,
Part 4 (EIC7700X_SoC_Technical_Reference_Manual_Part4.pdf):
https://github.com/eswincomputing/EIC7700X-SoC-Technical-Reference-Manual/releases
There are currently no in-tree users of the EIC7700 Ethernet driver, so
these changes are safe.
Fixes: 888bd0eca93c ("dt-bindings: ethernet: eswin: Document for EIC7700 SoC")
Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
---
.../bindings/net/eswin,eic7700-eth.yaml | 75 +++++++++++++++----
1 file changed, 59 insertions(+), 16 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
index 91e8cd1db67b..22d1cecea07e 100644
--- a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
+++ b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
@@ -20,6 +20,7 @@ select:
contains:
enum:
- eswin,eic7700-qos-eth
+ - eswin,eic7700-qos-eth-clk-inversion
required:
- compatible
@@ -28,9 +29,13 @@ allOf:
properties:
compatible:
- items:
- - const: eswin,eic7700-qos-eth
- - const: snps,dwmac-5.20
+ oneOf:
+ - items:
+ - const: eswin,eic7700-qos-eth
+ - const: snps,dwmac-5.20
+ - items:
+ - const: eswin,eic7700-qos-eth-clk-inversion
+ - const: snps,dwmac-5.20
reg:
maxItems: 1
@@ -63,16 +68,29 @@ properties:
- const: stmmaceth
rx-internal-delay-ps:
- enum: [0, 200, 600, 1200, 1600, 1800, 2000, 2200, 2400]
+ minimum: 0
+ maximum: 2540
+ multipleOf: 20
tx-internal-delay-ps:
- enum: [0, 200, 600, 1200, 1600, 1800, 2000, 2200, 2400]
+ minimum: 0
+ maximum: 2540
+ multipleOf: 20
eswin,hsp-sp-csr:
description:
HSP CSR is to control and get status of different high-speed peripherals
(such as Ethernet, USB, SATA, etc.) via register, which can tune
board-level's parameters of PHY, etc.
+
+ Additional background information about the High-Speed Subsystem
+ and the HSP CSR block is available in Chapter 10 ("High-Speed Interface")
+ of the EIC7700X SoC Technical Reference Manual, Part 4
+ (EIC7700X_SoC_Technical_Reference_Manual_Part4.pdf). The manual is
+ publicly available at
+ https://github.com/eswincomputing/EIC7700X-SoC-Technical-Reference-Manual/releases
+
+ This reference is provided for background information only.
$ref: /schemas/types.yaml#/definitions/phandle-array
items:
- items:
@@ -81,7 +99,9 @@ properties:
or external clock selection
- description: Offset of AXI clock controller Low-Power request
register
+ - description: Offset of register controlling TXD delay
- description: Offset of register controlling TX/RX clock delay
+ - description: Offset of register controlling RXD delay
required:
- compatible
@@ -93,8 +113,6 @@ required:
- phy-mode
- resets
- reset-names
- - rx-internal-delay-ps
- - tx-internal-delay-ps
- eswin,hsp-sp-csr
unevaluatedProperties: false
@@ -104,24 +122,49 @@ examples:
ethernet@50400000 {
compatible = "eswin,eic7700-qos-eth", "snps,dwmac-5.20";
reg = <0x50400000 0x10000>;
+ interrupt-parent = <&plic>;
+ interrupts = <61>;
+ interrupt-names = "macirq";
clocks = <&d0_clock 186>, <&d0_clock 171>, <&d0_clock 40>,
<&d0_clock 193>;
clock-names = "axi", "cfg", "stmmaceth", "tx";
+ resets = <&reset 95>;
+ reset-names = "stmmaceth";
+ eswin,hsp-sp-csr = <&hsp_sp_csr 0x100 0x108 0x114 0x118 0x11c>;
+ phy-handle = <&gmac0_phy0>;
+ phy-mode = "rgmii-id";
+ snps,aal;
+ snps,fixed-burst;
+ snps,tso;
+ snps,axi-config = <&stmmac_axi_setup_gmac0>;
+
+ stmmac_axi_setup_gmac0: stmmac-axi-config {
+ snps,blen = <0 0 0 0 16 8 4>;
+ snps,rd_osr_lmt = <2>;
+ snps,wr_osr_lmt = <2>;
+ };
+ };
+
+ ethernet@50410000 {
+ compatible = "eswin,eic7700-qos-eth-clk-inversion", "snps,dwmac-5.20";
+ reg = <0x50410000 0x10000>;
interrupt-parent = <&plic>;
- interrupts = <61>;
+ interrupts = <70>;
interrupt-names = "macirq";
- phy-mode = "rgmii-id";
- phy-handle = <&phy0>;
- resets = <&reset 95>;
+ clocks = <&d0_clock 186>, <&d0_clock 171>, <&d0_clock 40>,
+ <&d0_clock 194>;
+ clock-names = "axi", "cfg", "stmmaceth", "tx";
+ resets = <&reset 94>;
reset-names = "stmmaceth";
- rx-internal-delay-ps = <200>;
- tx-internal-delay-ps = <200>;
- eswin,hsp-sp-csr = <&hsp_sp_csr 0x100 0x108 0x118>;
- snps,axi-config = <&stmmac_axi_setup>;
+ eswin,hsp-sp-csr = <&hsp_sp_csr 0x200 0x208 0x214 0x218 0x21c>;
+ phy-handle = <&gmac1_phy0>;
+ phy-mode = "rgmii-id";
snps,aal;
snps,fixed-burst;
snps,tso;
- stmmac_axi_setup: stmmac-axi-config {
+ snps,axi-config = <&stmmac_axi_setup_gmac1>;
+
+ stmmac_axi_setup_gmac1: stmmac-axi-config {
snps,blen = <0 0 0 0 16 8 4>;
snps,rd_osr_lmt = <2>;
snps,wr_osr_lmt = <2>;
--
2.25.1On Tue, Mar 03, 2026 at 02:16:37PM +0800, lizhi2@eswincomputing.com wrote:
> From: Zhi Li <lizhi2@eswincomputing.com>
>
> The second Ethernet controller (eth1) on the EIC7700 SoC may experience
> RX data sampling issues at high speed due to EIC7700-specific receive
> clock to data skew at the MAC input.
>
> On the EIC7700 SoC, the second Ethernet controller (eth1) requires
> inversion of the internal RGMII receive clock in order to meet RX data
> sampling timing at high speed.
>
> Describe this SoC-specific difference by introducing a distinct compatible
> string for MAC instances that require internal clock inversion, allowing the
> driver to select the appropriate configuration without relying on per-board
> vendor-specific properties.
Pointless description/paragrapgh. Your explanation why adding a
compatible is "because I need compatible". That's completely redundant.
Explain what is special about this MAC instance, what's different in its
programming model or other characteristics that you claim it is a
different device.
>
> The rx-internal-delay-ps and tx-internal-delay-ps properties now use
> minimum and maximum constraints to reflect the actual hardware delay
> range (0-2540 ps) applied in 20 ps steps. This relaxes the binding
> validation compared to the previous enum-based definition and avoids
> regressions for existing DTBs while keeping the same hardware limits.
>
> Treat the RX/TX internal delay properties as optional, board-specific
> tuning knobs and remove them from the example to avoid encouraging
> their use.
>
> In addition, the binding now includes additional background information
> about the HSP CSR registers accessed by the MAC. The TXD and RXD delay
> control registers are included so the driver can explicitly clear any
> residual configuration left by the bootloader. Background reference for
> the High-Speed Subsystem and HSP CSR block is available in Chapter 10
> ("High-Speed Interface") of the EIC7700X SoC Technical Reference Manual,
> Part 4 (EIC7700X_SoC_Technical_Reference_Manual_Part4.pdf):
> https://github.com/eswincomputing/EIC7700X-SoC-Technical-Reference-Manual/releases
>
> There are currently no in-tree users of the EIC7700 Ethernet driver, so
> these changes are safe.
>
> Fixes: 888bd0eca93c ("dt-bindings: ethernet: eswin: Document for EIC7700 SoC")
> Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
> ---
> .../bindings/net/eswin,eic7700-eth.yaml | 75 +++++++++++++++----
> 1 file changed, 59 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> index 91e8cd1db67b..22d1cecea07e 100644
> --- a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> +++ b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> @@ -20,6 +20,7 @@ select:
> contains:
> enum:
> - eswin,eic7700-qos-eth
> + - eswin,eic7700-qos-eth-clk-inversion
> required:
> - compatible
>
> @@ -28,9 +29,13 @@ allOf:
>
> properties:
> compatible:
> - items:
> - - const: eswin,eic7700-qos-eth
> - - const: snps,dwmac-5.20
> + oneOf:
> + - items:
> + - const: eswin,eic7700-qos-eth
> + - const: snps,dwmac-5.20
> + - items:
> + - const: eswin,eic7700-qos-eth-clk-inversion
So just enum for both entries?
Anyway, that's the same device, so you do not get two compatibles. This
should be a property. Which property not sure, maybe all this was
discussed already.
> + - const: snps,dwmac-5.20
>
> reg:
> maxItems: 1
> @@ -63,16 +68,29 @@ properties:
> - const: stmmaceth
>
> rx-internal-delay-ps:
> - enum: [0, 200, 600, 1200, 1600, 1800, 2000, 2200, 2400]
> + minimum: 0
> + maximum: 2540
> + multipleOf: 20
>
> tx-internal-delay-ps:
> - enum: [0, 200, 600, 1200, 1600, 1800, 2000, 2200, 2400]
> + minimum: 0
> + maximum: 2540
> + multipleOf: 20
>
> eswin,hsp-sp-csr:
> description:
> HSP CSR is to control and get status of different high-speed peripherals
> (such as Ethernet, USB, SATA, etc.) via register, which can tune
> board-level's parameters of PHY, etc.
> +
> + Additional background information about the High-Speed Subsystem
> + and the HSP CSR block is available in Chapter 10 ("High-Speed Interface")
> + of the EIC7700X SoC Technical Reference Manual, Part 4
> + (EIC7700X_SoC_Technical_Reference_Manual_Part4.pdf). The manual is
> + publicly available at
> + https://github.com/eswincomputing/EIC7700X-SoC-Technical-Reference-Manual/releases
> +
> + This reference is provided for background information only.
> $ref: /schemas/types.yaml#/definitions/phandle-array
> items:
> - items:
> @@ -81,7 +99,9 @@ properties:
> or external clock selection
> - description: Offset of AXI clock controller Low-Power request
> register
> + - description: Offset of register controlling TXD delay
> - description: Offset of register controlling TX/RX clock delay
> + - description: Offset of register controlling RXD delay
As pointed out, you cannot change the order and there is no reason for
doing this explained in commit msg.
Best regards,
Krzysztof
Hi Krzysztof,
On 3/3/26 23:44, Krzysztof Kozlowski wrote:
> On Tue, Mar 03, 2026 at 02:16:37PM +0800, lizhi2@eswincomputing.com wrote:
>> From: Zhi Li <lizhi2@eswincomputing.com>
>>
>> The second Ethernet controller (eth1) on the EIC7700 SoC may experience
>> RX data sampling issues at high speed due to EIC7700-specific receive
>> clock to data skew at the MAC input.
>>
>> On the EIC7700 SoC, the second Ethernet controller (eth1) requires
>> inversion of the internal RGMII receive clock in order to meet RX data
>> sampling timing at high speed.
>>
>> Describe this SoC-specific difference by introducing a distinct compatible
>> string for MAC instances that require internal clock inversion, allowing the
>> driver to select the appropriate configuration without relying on per-board
>> vendor-specific properties.
>
> Pointless description/paragrapgh. Your explanation why adding a
> compatible is "because I need compatible". That's completely redundant.
>
> Explain what is special about this MAC instance, what's different in its
> programming model or other characteristics that you claim it is a
> different device.
>
I think ESWIN should improve the description/paragraph and properly doc
the timing issues discussed here:
https://lore.kernel.org/lkml/32a1f814.2c79.19bfe173225.Coremail.linmin@eswincomputing.com/
I do feel the need of using a different compatible, though. I think we
discussed in depth in that thread (link above), and advice from Andrew
https://lore.kernel.org/lkml/59cec617-0189-4dc3-bc3f-6346155a62ae@lunn.ch/
https://lore.kernel.org/lkml/bd202cfa-d6eb-4d0e-982d-b49795dd25f7@lunn.ch/
is to basically apply different parameters to MAC based on eth0/eth1. The
compatible string approach is a clean solution to achieve that. The reason
being that for eth1, there's no way to meet the standard without clock
inversion, given the vast internal clock skew. I don't think claiming it
as a different device than eth0 is that far-fetched. Hence, no need for an
additional property and the driver code to check for that.
>>
>> The rx-internal-delay-ps and tx-internal-delay-ps properties now use
>> minimum and maximum constraints to reflect the actual hardware delay
>> range (0-2540 ps) applied in 20 ps steps. This relaxes the binding
>> validation compared to the previous enum-based definition and avoids
>> regressions for existing DTBs while keeping the same hardware limits.
>>
>> Treat the RX/TX internal delay properties as optional, board-specific
>> tuning knobs and remove them from the example to avoid encouraging
>> their use.
>>
>> In addition, the binding now includes additional background information
>> about the HSP CSR registers accessed by the MAC. The TXD and RXD delay
>> control registers are included so the driver can explicitly clear any
>> residual configuration left by the bootloader. Background reference for
>> the High-Speed Subsystem and HSP CSR block is available in Chapter 10
>> ("High-Speed Interface") of the EIC7700X SoC Technical Reference Manual,
>> Part 4 (EIC7700X_SoC_Technical_Reference_Manual_Part4.pdf):
>> https://github.com/eswincomputing/EIC7700X-SoC-Technical-Reference-Manual/releases
>>
>> There are currently no in-tree users of the EIC7700 Ethernet driver, so
>> these changes are safe.
>>
>> Fixes: 888bd0eca93c ("dt-bindings: ethernet: eswin: Document for EIC7700 SoC")
>> Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
>> ---
>> .../bindings/net/eswin,eic7700-eth.yaml | 75 +++++++++++++++----
>> 1 file changed, 59 insertions(+), 16 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
>> index 91e8cd1db67b..22d1cecea07e 100644
>> --- a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
>> +++ b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
>> @@ -20,6 +20,7 @@ select:
>> contains:
>> enum:
>> - eswin,eic7700-qos-eth
>> + - eswin,eic7700-qos-eth-clk-inversion
>> required:
>> - compatible
>>
>> @@ -28,9 +29,13 @@ allOf:
>>
>> properties:
>> compatible:
>> - items:
>> - - const: eswin,eic7700-qos-eth
>> - - const: snps,dwmac-5.20
>> + oneOf:
>> + - items:
>> + - const: eswin,eic7700-qos-eth
>> + - const: snps,dwmac-5.20
>> + - items:
>> + - const: eswin,eic7700-qos-eth-clk-inversion
>
> So just enum for both entries?
>
> Anyway, that's the same device, so you do not get two compatibles. This
> should be a property. Which property not sure, maybe all this was
> discussed already.
>
>
>> + - const: snps,dwmac-5.20
>>
>> reg:
>> maxItems: 1
>> @@ -63,16 +68,29 @@ properties:
>> - const: stmmaceth
>>
>> rx-internal-delay-ps:
>> - enum: [0, 200, 600, 1200, 1600, 1800, 2000, 2200, 2400]
>> + minimum: 0
>> + maximum: 2540
>> + multipleOf: 20
>>
>> tx-internal-delay-ps:
>> - enum: [0, 200, 600, 1200, 1600, 1800, 2000, 2200, 2400]
>> + minimum: 0
>> + maximum: 2540
>> + multipleOf: 20
>>
>> eswin,hsp-sp-csr:
>> description:
>> HSP CSR is to control and get status of different high-speed peripherals
>> (such as Ethernet, USB, SATA, etc.) via register, which can tune
>> board-level's parameters of PHY, etc.
>> +
>> + Additional background information about the High-Speed Subsystem
>> + and the HSP CSR block is available in Chapter 10 ("High-Speed Interface")
>> + of the EIC7700X SoC Technical Reference Manual, Part 4
>> + (EIC7700X_SoC_Technical_Reference_Manual_Part4.pdf). The manual is
>> + publicly available at
>> + https://github.com/eswincomputing/EIC7700X-SoC-Technical-Reference-Manual/releases
>> +
>> + This reference is provided for background information only.
>> $ref: /schemas/types.yaml#/definitions/phandle-array
>> items:
>> - items:
>> @@ -81,7 +99,9 @@ properties:
>> or external clock selection
>> - description: Offset of AXI clock controller Low-Power request
>> register
>> + - description: Offset of register controlling TXD delay
>> - description: Offset of register controlling TX/RX clock delay
>> + - description: Offset of register controlling RXD delay
>
> As pointed out, you cannot change the order and there is no reason for
> doing this explained in commit msg.
>
> Best regards,
> Krzysztof
>
Bo
On Tue, 3 Mar 2026 14:16:37 +0800 lizhi2@eswincomputing.com wrote:
> There are currently no in-tree users of the EIC7700 Ethernet driver, so
> these changes are safe.
What do you mean by this sentence? The commit under Fixes was part of
Linux v6.19 already.
> Fixes: 888bd0eca93c ("dt-bindings: ethernet: eswin: Document for EIC7700 SoC")
On Tue, Mar 03, 2026 at 04:38:46PM -0800, Jakub Kicinski wrote:
> On Tue, 3 Mar 2026 14:16:37 +0800 lizhi2@eswincomputing.com wrote:
> > There are currently no in-tree users of the EIC7700 Ethernet driver, so
> > these changes are safe.
>
> What do you mean by this sentence? The commit under Fixes was part of
> Linux v6.19 already.
The "funny" thing is that caring about users doesn't even really matter
on the devicetree patch, except for this hunk:
|@@ -81,7 +99,9 @@ properties:
| or external clock selection
| - description: Offset of AXI clock controller Low-Power request
| register
|+ - description: Offset of register controlling TXD delay
| - description: Offset of register controlling TX/RX clock delay
|+ - description: Offset of register controlling RXD delay
|
| required:
| - compatible
And it only matters here because an item is injected mid-list. If this
was moved to the end with the RXD delay, the **dt-binding** changes
don't have issues with safety. I've not looked at whether there are
knock-on concerns about users in the driver or whatever yet, but from a
binding POV only that hunk can break something that currently works.
> > Fixes: 888bd0eca93c ("dt-bindings: ethernet: eswin: Document for EIC7700 SoC")
Hi All,
On 3/3/26 16:47, Conor Dooley wrote:
> On Tue, Mar 03, 2026 at 04:38:46PM -0800, Jakub Kicinski wrote:
>> On Tue, 3 Mar 2026 14:16:37 +0800 lizhi2@eswincomputing.com wrote:
>>> There are currently no in-tree users of the EIC7700 Ethernet driver, so
>>> these changes are safe.
>>
>> What do you mean by this sentence? The commit under Fixes was part of
>> Linux v6.19 already.
>
> The "funny" thing is that caring about users doesn't even really matter
> on the devicetree patch, except for this hunk:
> |@@ -81,7 +99,9 @@ properties:
> | or external clock selection
> | - description: Offset of AXI clock controller Low-Power request
> | register
> |+ - description: Offset of register controlling TXD delay
> | - description: Offset of register controlling TX/RX clock delay
> |+ - description: Offset of register controlling RXD delay
> |
> | required:
> | - compatible
> And it only matters here because an item is injected mid-list. If this
> was moved to the end with the RXD delay, the **dt-binding** changes
> don't have issues with safety. I've not looked at whether there are
> knock-on concerns about users in the driver or whatever yet, but from a
> binding POV only that hunk can break something that currently works.
This was already discussed here in v1:
https://lore.kernel.org/lkml/e7183ae1-8b8b-4e77-9f4e-3bc1b4b63556@lunn.ch/
The device-tree is not checked in yet by ESWIN folks, so there's currently
no user of the dt-binding. No need to worry about backward compat.
>
>>> Fixes: 888bd0eca93c ("dt-bindings: ethernet: eswin: Document for EIC7700 SoC")
Bo
On Tue, Mar 03, 2026 at 05:23:18PM -0800, Bo Gan wrote: > Hi All, > > On 3/3/26 16:47, Conor Dooley wrote: > > On Tue, Mar 03, 2026 at 04:38:46PM -0800, Jakub Kicinski wrote: > > > On Tue, 3 Mar 2026 14:16:37 +0800 lizhi2@eswincomputing.com wrote: > > > > There are currently no in-tree users of the EIC7700 Ethernet driver, so > > > > these changes are safe. > > > > > > What do you mean by this sentence? The commit under Fixes was part of > > > Linux v6.19 already. > > > > The "funny" thing is that caring about users doesn't even really matter > > on the devicetree patch, except for this hunk: > > |@@ -81,7 +99,9 @@ properties: > > | or external clock selection > > | - description: Offset of AXI clock controller Low-Power request > > | register > > |+ - description: Offset of register controlling TXD delay > > | - description: Offset of register controlling TX/RX clock delay > > |+ - description: Offset of register controlling RXD delay > > | > > | required: > > | - compatible > > And it only matters here because an item is injected mid-list. If this > > was moved to the end with the RXD delay, the **dt-binding** changes > > don't have issues with safety. I've not looked at whether there are > > knock-on concerns about users in the driver or whatever yet, but from a > > binding POV only that hunk can break something that currently works. > > This was already discussed here in v1: > https://lore.kernel.org/lkml/e7183ae1-8b8b-4e77-9f4e-3bc1b4b63556@lunn.ch/ > > The device-tree is not checked in yet by ESWIN folks, so there's currently > no user of the dt-binding. No need to worry about backward compat. The binding and driver exist, there doesn't need to be a dts in tree for there to be potential users. If the break was important I might not care, but this seems to be a gratuitous break, since the new items could be added to the end of the list and compatibility maintained without incurring any more difficulty for you.
> -----原始邮件----- > 发件人: "Conor Dooley" <conor@kernel.org> > 发送时间:2026-03-04 17:30:57 (星期三) > 收件人: "Bo Gan" <ganboing@gmail.com> > 抄送: "Jakub Kicinski" <kuba@kernel.org>, lizhi2@eswincomputing.com, devicetree@vger.kernel.org, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, netdev@vger.kernel.org, pabeni@redhat.com, mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com, rmk+kernel@armlinux.org.uk, wens@kernel.org, pjw@kernel.org, palmer@dabbelt.com, aou@eecs.berkeley.edu, alex@ghiti.fr, linux-riscv@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com, pritesh.patel@einfochips.com, weishangjuan@eswincomputing.com > 主题: Re: [PATCH net-next v3 1/3] dt-bindings: ethernet: eswin: add clock sampling control > > On Tue, Mar 03, 2026 at 05:23:18PM -0800, Bo Gan wrote: > > Hi All, > > > > On 3/3/26 16:47, Conor Dooley wrote: > > > On Tue, Mar 03, 2026 at 04:38:46PM -0800, Jakub Kicinski wrote: > > > > On Tue, 3 Mar 2026 14:16:37 +0800 lizhi2@eswincomputing.com wrote: > > > > > There are currently no in-tree users of the EIC7700 Ethernet driver, so > > > > > these changes are safe. > > > > > > > > What do you mean by this sentence? The commit under Fixes was part of > > > > Linux v6.19 already. > > > > > > The "funny" thing is that caring about users doesn't even really matter > > > on the devicetree patch, except for this hunk: > > > |@@ -81,7 +99,9 @@ properties: > > > | or external clock selection > > > | - description: Offset of AXI clock controller Low-Power request > > > | register > > > |+ - description: Offset of register controlling TXD delay > > > | - description: Offset of register controlling TX/RX clock delay > > > |+ - description: Offset of register controlling RXD delay > > > | > > > | required: > > > | - compatible > > > And it only matters here because an item is injected mid-list. If this > > > was moved to the end with the RXD delay, the **dt-binding** changes > > > don't have issues with safety. I've not looked at whether there are > > > knock-on concerns about users in the driver or whatever yet, but from a > > > binding POV only that hunk can break something that currently works. > > > > This was already discussed here in v1: > > https://lore.kernel.org/lkml/e7183ae1-8b8b-4e77-9f4e-3bc1b4b63556@lunn.ch/ > > > > The device-tree is not checked in yet by ESWIN folks, so there's currently > > no user of the dt-binding. No need to worry about backward compat. > > The binding and driver exist, there doesn't need to be a dts in tree for > there to be potential users. If the break was important I might not > care, but this seems to be a gratuitous break, since the new items could > be added to the end of the list and compatibility maintained without > incurring any more difficulty for you. Hi Conor and Krzysztof, Thanks for the reviews. - The next patch will fix the property order to avoid any breakage with existing DT bindings. - Eth1 does have a timing issue in silicon, as discussed here: https://lore.kernel.org/lkml/32a1f814.2c79.19bfe173225.Coremail.linmin@eswincomputing.com/ Based on this, and according to the advice from Andrew https://lore.kernel.org/lkml/59cec617-0189-4dc3-bc3f-6346155a62ae@lunn.ch/ https://lore.kernel.org/lkml/bd202cfa-d6eb-4d0e-982d-b49795dd25f7@lunn.ch/ adding a DT property is not a reasonable approach. In the next patch, I will improve the description/paragraph and properly document the timing issues. Do you think this is okay? Best regards, Zhi Li
On Thu, Mar 05, 2026 at 10:52:38AM +0800, 李志 wrote: > > > > > -----原始邮件----- > > 发件人: "Conor Dooley" <conor@kernel.org> > > 发送时间:2026-03-04 17:30:57 (星期三) > > 收件人: "Bo Gan" <ganboing@gmail.com> > > 抄送: "Jakub Kicinski" <kuba@kernel.org>, lizhi2@eswincomputing.com, devicetree@vger.kernel.org, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, netdev@vger.kernel.org, pabeni@redhat.com, mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com, rmk+kernel@armlinux.org.uk, wens@kernel.org, pjw@kernel.org, palmer@dabbelt.com, aou@eecs.berkeley.edu, alex@ghiti.fr, linux-riscv@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com, pritesh.patel@einfochips.com, weishangjuan@eswincomputing.com > > 主题: Re: [PATCH net-next v3 1/3] dt-bindings: ethernet: eswin: add clock sampling control > > > > On Tue, Mar 03, 2026 at 05:23:18PM -0800, Bo Gan wrote: > > > Hi All, > > > > > > On 3/3/26 16:47, Conor Dooley wrote: > > > > On Tue, Mar 03, 2026 at 04:38:46PM -0800, Jakub Kicinski wrote: > > > > > On Tue, 3 Mar 2026 14:16:37 +0800 lizhi2@eswincomputing.com wrote: > > > > > > There are currently no in-tree users of the EIC7700 Ethernet driver, so > > > > > > these changes are safe. > > > > > > > > > > What do you mean by this sentence? The commit under Fixes was part of > > > > > Linux v6.19 already. > > > > > > > > The "funny" thing is that caring about users doesn't even really matter > > > > on the devicetree patch, except for this hunk: > > > > |@@ -81,7 +99,9 @@ properties: > > > > | or external clock selection > > > > | - description: Offset of AXI clock controller Low-Power request > > > > | register > > > > |+ - description: Offset of register controlling TXD delay > > > > | - description: Offset of register controlling TX/RX clock delay > > > > |+ - description: Offset of register controlling RXD delay > > > > | > > > > | required: > > > > | - compatible > > > > And it only matters here because an item is injected mid-list. If this > > > > was moved to the end with the RXD delay, the **dt-binding** changes > > > > don't have issues with safety. I've not looked at whether there are > > > > knock-on concerns about users in the driver or whatever yet, but from a > > > > binding POV only that hunk can break something that currently works. > > > > > > This was already discussed here in v1: > > > https://lore.kernel.org/lkml/e7183ae1-8b8b-4e77-9f4e-3bc1b4b63556@lunn.ch/ > > > > > > The device-tree is not checked in yet by ESWIN folks, so there's currently > > > no user of the dt-binding. No need to worry about backward compat. > > > > The binding and driver exist, there doesn't need to be a dts in tree for > > there to be potential users. If the break was important I might not > > care, but this seems to be a gratuitous break, since the new items could > > be added to the end of the list and compatibility maintained without > > incurring any more difficulty for you. > > Hi Conor and Krzysztof, > > Thanks for the reviews. > > - The next patch will fix the property order to avoid any breakage > with existing DT bindings. Good, thanks. > - Eth1 does have a timing issue in silicon, as discussed here: > https://lore.kernel.org/lkml/32a1f814.2c79.19bfe173225.Coremail.linmin@eswincomputing.com/ > > Based on this, and according to the advice from Andrew > https://lore.kernel.org/lkml/59cec617-0189-4dc3-bc3f-6346155a62ae@lunn.ch/ > https://lore.kernel.org/lkml/bd202cfa-d6eb-4d0e-982d-b49795dd25f7@lunn.ch/ > adding a DT property is not a reasonable approach. > > In the next patch, I will improve the description/paragraph and properly > document the timing issues. I personally don't mind having two compatibles, but I might be more clear about what device the new one refers to (so something like s/clk-inversion/eth1/g). But Krzysztof was the one who objected to having multiple compatibles, so it's worth waiting to see what he has to say.
On Tue, Mar 03, 2026 at 05:23:18PM -0800, Bo Gan wrote: > Hi All, > > On 3/3/26 16:47, Conor Dooley wrote: > > On Tue, Mar 03, 2026 at 04:38:46PM -0800, Jakub Kicinski wrote: > > > On Tue, 3 Mar 2026 14:16:37 +0800 lizhi2@eswincomputing.com wrote: > > > > There are currently no in-tree users of the EIC7700 Ethernet driver, so > > > > these changes are safe. > > > > > > What do you mean by this sentence? The commit under Fixes was part of > > > Linux v6.19 already. > > > > The "funny" thing is that caring about users doesn't even really matter > > on the devicetree patch, except for this hunk: > > |@@ -81,7 +99,9 @@ properties: > > | or external clock selection > > | - description: Offset of AXI clock controller Low-Power request > > | register > > |+ - description: Offset of register controlling TXD delay > > | - description: Offset of register controlling TX/RX clock delay > > |+ - description: Offset of register controlling RXD delay > > | > > | required: > > | - compatible > > And it only matters here because an item is injected mid-list. If this > > was moved to the end with the RXD delay, the **dt-binding** changes > > don't have issues with safety. I've not looked at whether there are > > knock-on concerns about users in the driver or whatever yet, but from a > > binding POV only that hunk can break something that currently works. > > This was already discussed here in v1: > https://lore.kernel.org/lkml/e7183ae1-8b8b-4e77-9f4e-3bc1b4b63556@lunn.ch/ > > The device-tree is not checked in yet by ESWIN folks, so there's currently > no user of the dt-binding. No need to worry about backward compat. Of course there is user of this binding, for example ESWIN. Or many other vendors using it out of tree. You documented ABI. Best regards, Krzysztof
© 2016 - 2026 Red Hat, Inc.