[PATCH net-next v4 0/6] net: hisi-femac: add support for Hi3798MV200, remove unmaintained compatibles

[PATCH net-next v4 0/6] net: hisi-femac: add support for Hi3798MV200, remove unmaintained compatibles

[Yang Xiwen via B4 Relay]

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
Changes in v4:
- edit commit log to show why mdio bus clk is optional (Krzysztof Kozlowski)
- add clk_bulk_disable_unprepare() during error path (Maxime Chevallier)
- remove of_node_put() (Simon Horman)
- remove histb-clock.h header in binding example as it's goign to be deprecated.
- rearrange patches so binding comes before driver
- Link to v3: https://lore.kernel.org/r/20240220-net-v3-0-b68e5b75e765@outlook.com

Changes in v3:
- rearrange patches to fix bot error. (Rob Herring)
- rewrite commit logs (Andrew Lunn)
- use clk_bulk_ APIs (Andrew Lunn)
- fix uninitialization use of ret (assign to -ENODEV before goto) (Simon Horman)
- Link to v2: https://lore.kernel.org/r/20240216-net-v2-0-89bd4b7065c2@outlook.com

Changes in v2:
- replace email.
- hisi-femac: s/BUS/MACIF (Andrew Lunn)
- hisi-femac: add "hisilicon,hisi-femac" compatible since the driver
  seems generic enough for various SoCs
- hisi-femac-mdio: convert binding to YAML (Krzysztof Kozlowski)
- rewrite commit logs (Krzysztof Kozlowski)
- Link to v1: https://lore.kernel.org/r/20240216-net-v1-0-e0ad972cda99@outlook.com

---
Yang Xiwen (6):
      dt-bindings: net: hisilicon-femac-mdio: convert to YAML
      net: mdio: hisi-femac: make clock optional
      dt-bindings: net: remove outdated hisilicon-femac
      dt-bindings: net: add hisilicon,hisi-femac
      net: hisilicon: add support for hisi_femac core on Hi3798MV200
      net: hisi_femac: remove unused compatible strings

 .../bindings/net/hisilicon,hisi-femac-mdio.yaml    |  43 ++++++++
 .../bindings/net/hisilicon,hisi-femac.yaml         | 116 +++++++++++++++++++++
 .../bindings/net/hisilicon-femac-mdio.txt          |  22 ----
 .../devicetree/bindings/net/hisilicon-femac.txt    |  41 --------
 drivers/net/ethernet/hisilicon/hisi_femac.c        |  78 +++++++++++---
 drivers/net/mdio/mdio-hisi-femac.c                 |   2 +-
 6 files changed, 221 insertions(+), 81 deletions(-)
---
base-commit: 8d3dea210042f54b952b481838c1e7dfc4ec751d
change-id: 20240216-net-9a208e17c40f

Best regards,
-- 
Yang Xiwen <forbidden405@outlook.com>

[PATCH net-next v4 1/6] dt-bindings: net: hisilicon-femac-mdio: convert to YAML

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

For some FEMAC cores, MDIO bus is integrated to the MAC controller. So
We don't have a dedicated MDIO bus clock.

Also due to the PHY reset procedure, it's required to manage all clocks
and resets in the MAC controller driver. MAC controller clock can not be
shared with MDIO bus node in dts.

Mark the clock optional to resolve this problem.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 .../bindings/net/hisilicon,hisi-femac-mdio.yaml    | 43 ++++++++++++++++++++++
 .../bindings/net/hisilicon-femac-mdio.txt          | 22 -----------
 2 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/hisilicon,hisi-femac-mdio.yaml b/Documentation/devicetree/bindings/net/hisilicon,hisi-femac-mdio.yaml
new file mode 100644
index 000000000000..ee8650ad98fc
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/hisilicon,hisi-femac-mdio.yaml
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/hisilicon,hisi-femac-mdio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiSilicon FEMAC MDIO bus
+
+maintainers:
+  - Yang Xiwen <forbidden405@formail.com>
+
+allOf:
+  - $ref: mdio.yaml#
+
+properties:
+  compatible:
+    const: hisilicon,hisi-femac-mdio
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    mdio@10091100 {
+        compatible = "hisilicon,hisi-femac-mdio";
+        reg = <0x10091100 0x20>;
+        clocks = <&clk_mdio>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        phy@1 {
+            reg = <1>;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt b/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
deleted file mode 100644
index 23a39a309d17..000000000000
--- a/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
+++ /dev/null
@@ -1,22 +0,0 @@
-Hisilicon Fast Ethernet MDIO Controller interface
-
-Required properties:
-- compatible: should be "hisilicon,hisi-femac-mdio".
-- reg: address and length of the register set for the device.
-- clocks: A phandle to the reference clock for this device.
-
-- PHY subnode: inherits from phy binding [1]
-[1] Documentation/devicetree/bindings/net/phy.txt
-
-Example:
-mdio: mdio@10091100 {
-	compatible = "hisilicon,hisi-femac-mdio";
-	reg = <0x10091100 0x10>;
-	clocks = <&crg HI3516CV300_MDIO_CLK>;
-	#address-cells = <1>;
-	#size-cells = <0>;
-
-	phy0: phy@1 {
-		reg = <1>;
-	};
-};

-- 
2.43.0

Re: [PATCH net-next v4 1/6] dt-bindings: net: hisilicon-femac-mdio: convert to YAML

[Krzysztof Kozlowski]

On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> For some FEMAC cores, MDIO bus is integrated to the MAC controller. So
> We don't have a dedicated MDIO bus clock.

Hm, this means you miss compatibles for these different cores.

> 
> Also due to the PHY reset procedure, it's required to manage all clocks
> and resets in the MAC controller driver. MAC controller clock can not be
> shared with MDIO bus node in dts.
> 
> Mark the clock optional to resolve this problem.

I don't understand how making clock optional solves this problem. Clock
is still there. Whether driver handles it, does not affect the binding.

Best regards,
Krzysztof

Re: [PATCH net-next v4 1/6] dt-bindings: net: hisilicon-femac-mdio: convert to YAML

[Yang Xiwen]

On 2/23/2024 2:14 AM, Krzysztof Kozlowski wrote:
> On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> For some FEMAC cores, MDIO bus is integrated to the MAC controller. So
>> We don't have a dedicated MDIO bus clock.
> Hm, this means you miss compatibles for these different cores.


So you mean adding a new compatible like 
"hisilicon,hisi-femac-mdio-integrated" for these ones without dedicated 
clocks?


>
>> Also due to the PHY reset procedure, it's required to manage all clocks
>> and resets in the MAC controller driver. MAC controller clock can not be
>> shared with MDIO bus node in dts.
>>
>> Mark the clock optional to resolve this problem.
> I don't understand how making clock optional solves this problem. Clock
> is still there. Whether driver handles it, does not affect the binding.
>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen

Re: [PATCH net-next v4 1/6] dt-bindings: net: hisilicon-femac-mdio: convert to YAML

[Krzysztof Kozlowski]

On 22/02/2024 19:19, Yang Xiwen wrote:
> On 2/23/2024 2:14 AM, Krzysztof Kozlowski wrote:
>> On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote:
>>> From: Yang Xiwen <forbidden405@outlook.com>
>>>
>>> For some FEMAC cores, MDIO bus is integrated to the MAC controller. So
>>> We don't have a dedicated MDIO bus clock.
>> Hm, this means you miss compatibles for these different cores.
> 
> 
> So you mean adding a new compatible like 
> "hisilicon,hisi-femac-mdio-integrated" for these ones without dedicated 
> clocks?

This is a part of a SoC, right? So compatibles should be SoC specific,
thus I expect some hisilicon,SoC-femac-mdio.

Best regards,
Krzysztof

Re: [PATCH net-next v4 1/6] dt-bindings: net: hisilicon-femac-mdio: convert to YAML

[Yang Xiwen]

On 2/24/2024 6:01 PM, Krzysztof Kozlowski wrote:
> On 22/02/2024 19:19, Yang Xiwen wrote:
>> On 2/23/2024 2:14 AM, Krzysztof Kozlowski wrote:
>>> On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote:
>>>> From: Yang Xiwen <forbidden405@outlook.com>
>>>>
>>>> For some FEMAC cores, MDIO bus is integrated to the MAC controller. So
>>>> We don't have a dedicated MDIO bus clock.
>>> Hm, this means you miss compatibles for these different cores.
>>
>> So you mean adding a new compatible like
>> "hisilicon,hisi-femac-mdio-integrated" for these ones without dedicated
>> clocks?
> This is a part of a SoC, right? So compatibles should be SoC specific,
> thus I expect some hisilicon,SoC-femac-mdio.


Now i'm very sure the old binding is incorrect. I've downloaded TRM for 
Hi3516DV300. And found that it is, in fact, very similar to Hi3798MV200. 
There is no dedicated MDIO bus clock. the MDIO bus clock is always 
provided by the MAC controller. So here i can say MDIO bus is always 
integrated into the FEMAC Ethernet controller, not the SoC.


Maybe someday we can merge these two drivers together later. We don't 
need a MDIO bus node at all!


>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen

[PATCH net-next v4 2/6] net: mdio: hisi-femac: make clock optional

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

The clocks are optional.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 drivers/net/mdio/mdio-hisi-femac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mdio/mdio-hisi-femac.c b/drivers/net/mdio/mdio-hisi-femac.c
index 6703f626ee83..a2620b200381 100644
--- a/drivers/net/mdio/mdio-hisi-femac.c
+++ b/drivers/net/mdio/mdio-hisi-femac.c
@@ -93,7 +93,7 @@ static int hisi_femac_mdio_probe(struct platform_device *pdev)
 		goto err_out_free_mdiobus;
 	}
 
-	data->clk = devm_clk_get(&pdev->dev, NULL);
+	data->clk = devm_clk_get_optional(&pdev->dev, NULL);
 	if (IS_ERR(data->clk)) {
 		ret = PTR_ERR(data->clk);
 		goto err_out_free_mdiobus;

-- 
2.43.0

[PATCH net-next v4 3/6] dt-bindings: net: remove outdated hisilicon-femac

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

The user documented(Hi3516) is not found in current kernel anymore. And
it seems this SoC has been EOL for a long time. While at it, Remove this
binding entirely due to recent driver changes.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 .../devicetree/bindings/net/hisilicon-femac.txt    | 41 ----------------------
 1 file changed, 41 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac.txt b/Documentation/devicetree/bindings/net/hisilicon-femac.txt
deleted file mode 100644
index 5f96976f3cea..000000000000
--- a/Documentation/devicetree/bindings/net/hisilicon-femac.txt
+++ /dev/null
@@ -1,41 +0,0 @@
-Hisilicon Fast Ethernet MAC controller
-
-Required properties:
-- compatible: should contain one of the following version strings:
-	* "hisilicon,hisi-femac-v1"
-	* "hisilicon,hisi-femac-v2"
-	and the soc string "hisilicon,hi3516cv300-femac".
-- reg: specifies base physical address(s) and size of the device registers.
-  The first region is the MAC core register base and size.
-  The second region is the global MAC control register.
-- interrupts: should contain the MAC interrupt.
-- clocks: A phandle to the MAC main clock.
-- resets: should contain the phandle to the MAC reset signal(required) and
-	the PHY reset signal(optional).
-- reset-names: should contain the reset signal name "mac"(required)
-	and "phy"(optional).
-- phy-mode: see ethernet.txt [1].
-- phy-handle: see ethernet.txt [1].
-- hisilicon,phy-reset-delays-us: triplet of delays if PHY reset signal given.
-	The 1st cell is reset pre-delay in micro seconds.
-	The 2nd cell is reset pulse in micro seconds.
-	The 3rd cell is reset post-delay in micro seconds.
-
-The MAC address will be determined using the optional properties
-defined in ethernet.txt[1].
-
-[1] Documentation/devicetree/bindings/net/ethernet.txt
-
-Example:
-	hisi_femac: ethernet@10090000 {
-		compatible = "hisilicon,hi3516cv300-femac","hisilicon,hisi-femac-v2";
-		reg = <0x10090000 0x1000>,<0x10091300 0x200>;
-		interrupts = <12>;
-		clocks = <&crg HI3518EV200_ETH_CLK>;
-		resets = <&crg 0xec 0>,<&crg 0xec 3>;
-		reset-names = "mac","phy";
-		mac-address = [00 00 00 00 00 00];
-		phy-mode = "mii";
-		phy-handle = <&phy0>;
-		hisilicon,phy-reset-delays-us = <10000 20000 20000>;
-	};

-- 
2.43.0

Re: [PATCH net-next v4 3/6] dt-bindings: net: remove outdated hisilicon-femac

[Krzysztof Kozlowski]

On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> The user documented(Hi3516) is not found in current kernel anymore. And
> it seems this SoC has been EOL for a long time. While at it, Remove this
> binding entirely due to recent driver changes.

This removes documented compatibles, which:
1. should cause checkpatch issues,
2. is not what we want.

Driver is still there, so first the driver would have to be removed...
but then why do you add it immediately back? No, it does not work like that.

Best regards,
Krzysztof

Re: [PATCH net-next v4 3/6] dt-bindings: net: remove outdated hisilicon-femac

[Yang Xiwen]

On 2/26/2024 3:50 PM, Krzysztof Kozlowski wrote:
> On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> The user documented(Hi3516) is not found in current kernel anymore. And
>> it seems this SoC has been EOL for a long time. While at it, Remove this
>> binding entirely due to recent driver changes.
> 
> This removes documented compatibles, which:
> 1. should cause checkpatch issues,
> 2. is not what we want.
> 
> Driver is still there, so first the driver would have to be removed...
> but then why do you add it immediately back? No, it does not work like that.

I has downloaded TRM for this old Hi3519 SoC. So i think with the new
driver, it still works for Hi3519 FEMAC controller. So i'll rewrite
these 3 patches and describe the whole thing in commit logs. i.e. the
old binding is wrong, inconsistent with real hardware, etc..

So you mean conversion to YAML first, in a separate commit. And then do
necessary edition to match the real hardware? I believe Hi3519 FEMAC and
Hi3798MV200 FEMAC are compatible in hardware. So the old binding is
wrong, missing critical hardware resources.

> 
> Best regards,
> Krzysztof
> 

-- 
Best regards,
Yang Xiwen

Re: [PATCH net-next v4 3/6] dt-bindings: net: remove outdated hisilicon-femac

[Krzysztof Kozlowski]

On 27/02/2024 02:43, Yang Xiwen wrote:
> On 2/26/2024 3:50 PM, Krzysztof Kozlowski wrote:
>> On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote:
>>> From: Yang Xiwen <forbidden405@outlook.com>
>>>
>>> The user documented(Hi3516) is not found in current kernel anymore. And
>>> it seems this SoC has been EOL for a long time. While at it, Remove this
>>> binding entirely due to recent driver changes.
>>
>> This removes documented compatibles, which:
>> 1. should cause checkpatch issues,
>> 2. is not what we want.
>>
>> Driver is still there, so first the driver would have to be removed...
>> but then why do you add it immediately back? No, it does not work like that.
> 
> I has downloaded TRM for this old Hi3519 SoC. So i think with the new
> driver, it still works for Hi3519 FEMAC controller. So i'll rewrite
> these 3 patches and describe the whole thing in commit logs. i.e. the
> old binding is wrong, inconsistent with real hardware, etc..
> 
> So you mean conversion to YAML first, in a separate commit. And then do
> necessary edition to match the real hardware? I believe Hi3519 FEMAC and
> Hi3798MV200 FEMAC are compatible in hardware. So the old binding is
> wrong, missing critical hardware resources.

If you remove binding, then it has to be done in reverse order than
adding it (see DT submitting patches). However removing binding and
re-adding it, is not what you think. It's a obfuscated way to make a
change, so better just make a change and explicitly say why and what is
happening

Best regards,
Krzysztof

[PATCH net-next v4 4/6] dt-bindings: net: add hisilicon,hisi-femac

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

Compared to previous txt based binding doc, the following changes are
made according to the TRM:

- Remove unmaintained Hi3516 SoC, add Hi3798MV200
- add MDIO subnode, because MDIO bus is integrated
- add ahb bus clock, phy clock and reset

Also remove "hisi-femac-v1/2" binding.

The difference between versions is unknown and not documented anywhere.
Nor is it used in driver. Remove it until it's needed in the future.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 .../bindings/net/hisilicon,hisi-femac.yaml         | 116 +++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/hisilicon,hisi-femac.yaml b/Documentation/devicetree/bindings/net/hisilicon,hisi-femac.yaml
new file mode 100644
index 000000000000..56d7c8a26d0c
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/hisilicon,hisi-femac.yaml
@@ -0,0 +1,116 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/hisilicon,hisi-femac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Hisilicon Fast Ethernet MAC controller
+
+maintainers:
+  - Yang Xiwen <forbidden405@foxmail.com>
+
+allOf:
+  - $ref: ethernet-controller.yaml
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - hisilicon,hi3798mv200-femac
+      - const: hisilicon,hisi-femac
+
+  reg:
+    items:
+      - description: The first region is the MAC core register base and size.
+      - description: The second region is the global MAC control register.
+
+  ranges:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 3
+
+  clock-names:
+    items:
+      - const: mac
+      - const: macif
+      - const: phy
+
+  resets:
+    maxItems: 2
+
+  reset-names:
+    items:
+      - const: mac
+      - const: phy
+
+  hisilicon,phy-reset-delays-us:
+    items:
+      - description: The 1st cell is reset pre-delay in micro seconds.
+      - description: The 2nd cell is reset pulse in micro seconds.
+      - description: The 3rd cell is reset post-delay in micro seconds.
+
+patternProperties:
+  '^mdio@[0-9a-f]+$':
+    $ref: hisilicon,hisi-femac-mdio.yaml#
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - phy-connection-type
+  - phy-handle
+  - hisilicon,phy-reset-delays-us
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    ethernet@9c30000 {
+        compatible = "hisilicon,hi3798mv200-femac", "hisilicon,hisi-femac";
+        reg = <0x9c30000 0x1000>, <0x9c31300 0x200>;
+        ranges = <0x0 0x9c30000 0x10000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+        interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clk_femac>,
+                 <&clk_femacif>,
+                 <&clk_fephy>;
+        clock-names = "mac", "macif", "phy";
+        resets = <&crg 0xd0 3>, <&crg 0x388 4>;
+        reset-names = "mac", "phy";
+        phy-handle = <&fephy>;
+        phy-connection-type = "mii";
+        // To be filled by bootloader
+        mac-address = [00 00 00 00 00 00];
+        hisilicon,phy-reset-delays-us = <10000 10000 500000>;
+        status = "okay";
+
+        mdio@1100 {
+            compatible = "hisilicon,hisi-femac-mdio";
+            reg = <0x1100 0x20>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            status = "okay";
+
+            ethernet-phy@1 {
+                reg = <1>;
+                #phy-cells = <0>;
+            };
+        };
+    };

-- 
2.43.0

Re: [PATCH net-next v4 4/6] dt-bindings: net: add hisilicon,hisi-femac

[Krzysztof Kozlowski]

On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> Compared to previous txt based binding doc, the following changes are
> made according to the TRM:
> 
> - Remove unmaintained Hi3516 SoC, add Hi3798MV200
> - add MDIO subnode, because MDIO bus is integrated
> - add ahb bus clock, phy clock and reset
> 
> Also remove "hisi-femac-v1/2" binding.

Where?

> 
> The difference between versions is unknown and not documented anywhere.
> Nor is it used in driver. Remove it until it's needed in the future.
> 
> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> ---
>  .../bindings/net/hisilicon,hisi-femac.yaml         | 116 +++++++++++++++++++++
>  1 file changed, 116 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/hisilicon,hisi-femac.yaml b/Documentation/devicetree/bindings/net/hisilicon,hisi-femac.yaml
> new file mode 100644
> index 000000000000..56d7c8a26d0c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/hisilicon,hisi-femac.yaml
> @@ -0,0 +1,116 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/hisilicon,hisi-femac.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Hisilicon Fast Ethernet MAC controller
> +
> +maintainers:
> +  - Yang Xiwen <forbidden405@foxmail.com>
> +
> +allOf:
> +  - $ref: ethernet-controller.yaml
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - hisilicon,hi3798mv200-femac
> +      - const: hisilicon,hisi-femac

If you were re-designing schema, then this should be dropped. No generic
compatibles which even the author does not understand what it is.

But if you re-use existing binding, then it is a proof that previous
commit did something odd.

> +
> +  reg:
> +    items:
> +      - description: The first region is the MAC core register base and size.
> +      - description: The second region is the global MAC control register.
> +
> +  ranges:
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 3
> +
> +  clock-names:
> +    items:
> +      - const: mac
> +      - const: macif
> +      - const: phy
> +
> +  resets:
> +    maxItems: 2
> +
> +  reset-names:
> +    items:
> +      - const: mac
> +      - const: phy
> +
> +  hisilicon,phy-reset-delays-us:
> +    items:
> +      - description: The 1st cell is reset pre-delay in micro seconds.

Drop redundant parts: "The 1st cell" and "micro seconds" and instead
describe something useful. You have here nine words out of which 7 are
redundant or not describing actual property and only two say something.
What is "reset pre-delay"? Explain this.

> +      - description: The 2nd cell is reset pulse in micro seconds.
> +      - description: The 3rd cell is reset post-delay in micro seconds.
> +
> +patternProperties:
> +  '^mdio@[0-9a-f]+$':
> +    $ref: hisilicon,hisi-femac-mdio.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - resets
> +  - reset-names
> +  - phy-connection-type
> +  - phy-handle
> +  - hisilicon,phy-reset-delays-us
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    ethernet@9c30000 {
> +        compatible = "hisilicon,hi3798mv200-femac", "hisilicon,hisi-femac";
> +        reg = <0x9c30000 0x1000>, <0x9c31300 0x200>;
> +        ranges = <0x0 0x9c30000 0x10000>;
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&clk_femac>,
> +                 <&clk_femacif>,
> +                 <&clk_fephy>;
> +        clock-names = "mac", "macif", "phy";
> +        resets = <&crg 0xd0 3>, <&crg 0x388 4>;
> +        reset-names = "mac", "phy";
> +        phy-handle = <&fephy>;
> +        phy-connection-type = "mii";
> +        // To be filled by bootloader
> +        mac-address = [00 00 00 00 00 00];
> +        hisilicon,phy-reset-delays-us = <10000 10000 500000>;
> +        status = "okay";

Didn't you receive feedback here already?

Best regards,
Krzysztof

[PATCH net-next v4 5/6] net: hisilicon: add support for hisi_femac core on Hi3798MV200

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

Register the sub MDIO bus if it is found. Also implement the internal
PHY reset procedure as needed.

Note it's unable to put the MDIO bus node outside of MAC controller
(i.e. at the same level in the parent bus node). Because we need to
control all clocks and resets in FEMAC driver due to the phy reset
procedure. So the clocks can't be assigned to MDIO bus device, which is
an essential resource for the MDIO bus to work.

No backward compatibility is maintained since the only existing
user(Hi3516DV300) has not received any updates from HiSilicon for about
8 years already. And till today, no mainline dts for this SoC is found.
It seems unlikely that there are still existing mainline Hi3516DV300
users. The effort to maintain the old binding seems gain a little.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 drivers/net/ethernet/hisilicon/hisi_femac.c | 74 +++++++++++++++++++++++------
 1 file changed, 60 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
index 2406263c9dd3..eab91e011d11 100644
--- a/drivers/net/ethernet/hisilicon/hisi_femac.c
+++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
@@ -10,8 +10,10 @@
 #include <linux/etherdevice.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/reset.h>
 
@@ -97,6 +99,13 @@ enum phy_reset_delays {
 	DELAYS_NUM,
 };
 
+enum clk_type {
+	CLK_MAC,
+	CLK_MACIF,
+	CLK_PHY,
+	CLK_NUM,
+};
+
 struct hisi_femac_queue {
 	struct sk_buff **skb;
 	dma_addr_t *dma_phys;
@@ -108,7 +117,7 @@ struct hisi_femac_queue {
 struct hisi_femac_priv {
 	void __iomem *port_base;
 	void __iomem *glb_base;
-	struct clk *clk;
+	struct clk_bulk_data *clks;
 	struct reset_control *mac_rst;
 	struct reset_control *phy_rst;
 	u32 phy_reset_delays[DELAYS_NUM];
@@ -116,6 +125,7 @@ struct hisi_femac_priv {
 
 	struct device *dev;
 	struct net_device *ndev;
+	struct platform_device *mdio_pdev;
 
 	struct hisi_femac_queue txq;
 	struct hisi_femac_queue rxq;
@@ -693,6 +703,7 @@ static const struct net_device_ops hisi_femac_netdev_ops = {
 static void hisi_femac_core_reset(struct hisi_femac_priv *priv)
 {
 	reset_control_assert(priv->mac_rst);
+	usleep_range(200, 300);
 	reset_control_deassert(priv->mac_rst);
 }
 
@@ -712,6 +723,10 @@ static void hisi_femac_sleep_us(u32 time_us)
 
 static void hisi_femac_phy_reset(struct hisi_femac_priv *priv)
 {
+	/* MAC clock must be disabled before PHY reset
+	 */
+	clk_disable(priv->clks[CLK_MAC].clk);
+	clk_disable(priv->clks[CLK_MACIF].clk);
 	/* To make sure PHY hardware reset success,
 	 * we must keep PHY in deassert state first and
 	 * then complete the hardware reset operation
@@ -727,6 +742,9 @@ static void hisi_femac_phy_reset(struct hisi_femac_priv *priv)
 	reset_control_deassert(priv->phy_rst);
 	/* delay some time to ensure later MDIO access */
 	hisi_femac_sleep_us(priv->phy_reset_delays[POST_DELAY]);
+
+	clk_enable(priv->clks[CLK_MAC].clk);
+	clk_enable(priv->clks[CLK_MACIF].clk);
 }
 
 static void hisi_femac_port_init(struct hisi_femac_priv *priv)
@@ -768,11 +786,12 @@ static void hisi_femac_port_init(struct hisi_femac_priv *priv)
 static int hisi_femac_drv_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *node = dev->of_node;
+	struct device_node *node = dev->of_node, *mdio_np;
 	struct net_device *ndev;
 	struct hisi_femac_priv *priv;
 	struct phy_device *phy;
 	int ret;
+	bool mdio_registered = false;
 
 	ndev = alloc_etherdev(sizeof(*priv));
 	if (!ndev)
@@ -797,17 +816,16 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
 		goto out_free_netdev;
 	}
 
-	priv->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(priv->clk)) {
-		dev_err(dev, "failed to get clk\n");
-		ret = -ENODEV;
+	ret = devm_clk_bulk_get_all(&pdev->dev, &priv->clks);
+	if (ret < 0 || ret != CLK_NUM) {
+		dev_err(dev, "failed to get clk bulk: %d\n", ret);
 		goto out_free_netdev;
 	}
 
-	ret = clk_prepare_enable(priv->clk);
+	ret = clk_bulk_prepare_enable(CLK_NUM, priv->clks);
 	if (ret) {
-		dev_err(dev, "failed to enable clk %d\n", ret);
-		goto out_free_netdev;
+		dev_err(dev, "failed to enable clk bulk: %d\n", ret);
+		goto out_disable_clk;
 	}
 
 	priv->mac_rst = devm_reset_control_get(dev, "mac");
@@ -830,11 +848,29 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
 		hisi_femac_phy_reset(priv);
 	}
 
+	// Register the optional MDIO bus
+	for_each_available_child_of_node(node, mdio_np) {
+		if (of_node_name_prefix(mdio_np, "mdio")) {
+			priv->mdio_pdev = of_platform_device_create(mdio_np, NULL, dev);
+			of_node_put(mdio_np);
+			if (!priv->mdio_pdev) {
+				dev_err(dev, "failed to register MDIO bus device\n");
+				ret = -ENODEV;
+				goto out_disable_clk;
+			}
+			mdio_registered = true;
+			break;
+		}
+	}
+
+	if (!mdio_registered)
+		dev_warn(dev, "MDIO subnode not found. This is usually a bug.\n");
+
 	phy = of_phy_get_and_connect(ndev, node, hisi_femac_adjust_link);
 	if (!phy) {
 		dev_err(dev, "connect to PHY failed!\n");
 		ret = -ENODEV;
-		goto out_disable_clk;
+		goto out_unregister_mdio_bus;
 	}
 
 	phy_attached_print(phy, "phy_id=0x%.8lx, phy_mode=%s\n",
@@ -885,8 +921,10 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
 out_disconnect_phy:
 	netif_napi_del(&priv->napi);
 	phy_disconnect(phy);
+out_unregister_mdio_bus:
+	platform_device_unregister(priv->mdio_pdev);
 out_disable_clk:
-	clk_disable_unprepare(priv->clk);
+	clk_bulk_disable_unprepare(CLK_NUM, priv->clks);
 out_free_netdev:
 	free_netdev(ndev);
 
@@ -902,7 +940,8 @@ static void hisi_femac_drv_remove(struct platform_device *pdev)
 	unregister_netdev(ndev);
 
 	phy_disconnect(ndev->phydev);
-	clk_disable_unprepare(priv->clk);
+	platform_device_unregister(priv->mdio_pdev);
+	clk_bulk_disable_unprepare(CLK_NUM, priv->clks);
 	free_netdev(ndev);
 }
 
@@ -919,7 +958,7 @@ static int hisi_femac_drv_suspend(struct platform_device *pdev,
 		netif_device_detach(ndev);
 	}
 
-	clk_disable_unprepare(priv->clk);
+	clk_bulk_disable_unprepare(CLK_NUM, priv->clks);
 
 	return 0;
 }
@@ -928,8 +967,14 @@ static int hisi_femac_drv_resume(struct platform_device *pdev)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct hisi_femac_priv *priv = netdev_priv(ndev);
+	int ret;
+
+	ret = clk_bulk_prepare_enable(CLK_NUM, priv->clks);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enable clk bulk: %d\n", ret);
+		return ret;
+	}
 
-	clk_prepare_enable(priv->clk);
 	if (priv->phy_rst)
 		hisi_femac_phy_reset(priv);
 
@@ -948,6 +993,7 @@ static const struct of_device_id hisi_femac_match[] = {
 	{.compatible = "hisilicon,hisi-femac-v1",},
 	{.compatible = "hisilicon,hisi-femac-v2",},
 	{.compatible = "hisilicon,hi3516cv300-femac",},
+	{.compatible = "hisilicon,hi3798mv200-femac",},
 	{},
 };
 

-- 
2.43.0

Re: [PATCH net-next v4 5/6] net: hisilicon: add support for hisi_femac core on Hi3798MV200

[Krzysztof Kozlowski]

On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> Register the sub MDIO bus if it is found. Also implement the internal
> PHY reset procedure as needed.
> 


> +	ret = clk_bulk_prepare_enable(CLK_NUM, priv->clks);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable clk bulk: %d\n", ret);
> +		return ret;
> +	}
>  
> -	clk_prepare_enable(priv->clk);
>  	if (priv->phy_rst)
>  		hisi_femac_phy_reset(priv);
>  
> @@ -948,6 +993,7 @@ static const struct of_device_id hisi_femac_match[] = {
>  	{.compatible = "hisilicon,hisi-femac-v1",},
>  	{.compatible = "hisilicon,hisi-femac-v2",},
>  	{.compatible = "hisilicon,hi3516cv300-femac",},
> +	{.compatible = "hisilicon,hi3798mv200-femac",},

What's the point of growing the list? All are fully compatible according
to your match table.

Stop growing it and either express compatibility or use proper match
data, if they are not compatible.

Best regards,
Krzysztof

[PATCH net-next v4 6/6] net: hisi_femac: remove unused compatible strings

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

The only documented SoC Hi3516DV300 does not receive any updates from 8
years ago. With the recent driver changes, it unlikely works for this
SoC anymore. Remove the binding for this SoC.

Also it's hard to get the version number and it's unknown how the
version can be used. Remove them until it's really needed.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 drivers/net/ethernet/hisilicon/hisi_femac.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
index eab91e011d11..9466ca9da2bb 100644
--- a/drivers/net/ethernet/hisilicon/hisi_femac.c
+++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
@@ -990,9 +990,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev)
 #endif
 
 static const struct of_device_id hisi_femac_match[] = {
-	{.compatible = "hisilicon,hisi-femac-v1",},
-	{.compatible = "hisilicon,hisi-femac-v2",},
-	{.compatible = "hisilicon,hi3516cv300-femac",},
+	{.compatible = "hisilicon,hisi-femac",},
 	{.compatible = "hisilicon,hi3798mv200-femac",},
 	{},
 };

-- 
2.43.0

Re: [PATCH net-next v4 6/6] net: hisi_femac: remove unused compatible strings

[Krzysztof Kozlowski]

On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> The only documented SoC Hi3516DV300 does not receive any updates from 8
> years ago. With the recent driver changes, it unlikely works for this
> SoC anymore. Remove the binding for this SoC.
> 
> Also it's hard to get the version number and it's unknown how the
> version can be used. Remove them until it's really needed.
> 
> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> ---
>  drivers/net/ethernet/hisilicon/hisi_femac.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
> index eab91e011d11..9466ca9da2bb 100644
> --- a/drivers/net/ethernet/hisilicon/hisi_femac.c
> +++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
> @@ -990,9 +990,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev)
>  #endif
>  
>  static const struct of_device_id hisi_femac_match[] = {
> -	{.compatible = "hisilicon,hisi-femac-v1",},
> -	{.compatible = "hisilicon,hisi-femac-v2",},
> -	{.compatible = "hisilicon,hi3516cv300-femac",},
> +	{.compatible = "hisilicon,hisi-femac",},

What is happening here? Removal could be justified, but then order of
your patches is totally wrong. But that hisi-femac is a no-go or provide
proper rationale.

Best regards,
Krzysztof

Re: [PATCH net-next v4 6/6] net: hisi_femac: remove unused compatible strings

[Yang Xiwen]

On 2/26/2024 3:55 PM, Krzysztof Kozlowski wrote:
> On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> The only documented SoC Hi3516DV300 does not receive any updates from 8
>> years ago. With the recent driver changes, it unlikely works for this
>> SoC anymore. Remove the binding for this SoC.
>>
>> Also it's hard to get the version number and it's unknown how the
>> version can be used. Remove them until it's really needed.
>>
>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>> ---
>>  drivers/net/ethernet/hisilicon/hisi_femac.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
>> index eab91e011d11..9466ca9da2bb 100644
>> --- a/drivers/net/ethernet/hisilicon/hisi_femac.c
>> +++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
>> @@ -990,9 +990,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev)
>>  #endif
>>  
>>  static const struct of_device_id hisi_femac_match[] = {
>> -	{.compatible = "hisilicon,hisi-femac-v1",},
>> -	{.compatible = "hisilicon,hisi-femac-v2",},
>> -	{.compatible = "hisilicon,hi3516cv300-femac",},
>> +	{.compatible = "hisilicon,hisi-femac",},
> 
> What is happening here? Removal could be justified, but then order of
> your patches is totally wrong. But that hisi-femac is a no-go or provide
> proper rationale.

I don't understand exactly... In dts, we commonly have a SoC specific
compatible string first, generic compatible string the second. e.g.

compatible = "hisilicon,hi3798mv200-perictrl", "syscon", "simple-mfd".

So i think this is recommended. Or does it mean we only need them in
dts, not in the driver? The generic driver only needs a generic
compatible "hisilicon,hisi-femac" in all?

> 
> Best regards,
> Krzysztof
> 

-- 
Best regards,
Yang Xiwen

Re: [PATCH net-next v4 6/6] net: hisi_femac: remove unused compatible strings

[Krzysztof Kozlowski]

On 27/02/2024 02:51, Yang Xiwen wrote:
> On 2/26/2024 3:55 PM, Krzysztof Kozlowski wrote:
>> On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote:
>>> From: Yang Xiwen <forbidden405@outlook.com>
>>>
>>> The only documented SoC Hi3516DV300 does not receive any updates from 8
>>> years ago. With the recent driver changes, it unlikely works for this
>>> SoC anymore. Remove the binding for this SoC.
>>>
>>> Also it's hard to get the version number and it's unknown how the
>>> version can be used. Remove them until it's really needed.
>>>
>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>>> ---
>>>  drivers/net/ethernet/hisilicon/hisi_femac.c | 4 +---
>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
>>> index eab91e011d11..9466ca9da2bb 100644
>>> --- a/drivers/net/ethernet/hisilicon/hisi_femac.c
>>> +++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
>>> @@ -990,9 +990,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev)
>>>  #endif
>>>  
>>>  static const struct of_device_id hisi_femac_match[] = {
>>> -	{.compatible = "hisilicon,hisi-femac-v1",},
>>> -	{.compatible = "hisilicon,hisi-femac-v2",},
>>> -	{.compatible = "hisilicon,hi3516cv300-femac",},
>>> +	{.compatible = "hisilicon,hisi-femac",},
>>
>> What is happening here? Removal could be justified, but then order of
>> your patches is totally wrong. But that hisi-femac is a no-go or provide
>> proper rationale.
> 
> I don't understand exactly... In dts, we commonly have a SoC specific
> compatible string first, generic compatible string the second. e.g.
> 
> compatible = "hisilicon,hi3798mv200-perictrl", "syscon", "simple-mfd".

There is no generic compatible here. hi3798mv200 is soc.

> 
> So i think this is recommended. Or does it mean we only need them in

It is allowed for certain cases and recommended for even fewer ones. Do
you want to say you have fully discoverable features here and you do not
need any properties? Or you want to say that all possible hardware will
have exactly the same programming interface (registers etc)?

Best regards,
Krzysztof

Re: [PATCH net-next v4 6/6] net: hisi_femac: remove unused compatible strings

[Yang Xiwen]

On 2/27/2024 2:53 PM, Krzysztof Kozlowski wrote:
> On 27/02/2024 02:51, Yang Xiwen wrote:
>> On 2/26/2024 3:55 PM, Krzysztof Kozlowski wrote:
>>> On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote:
>>>> From: Yang Xiwen <forbidden405@outlook.com>
>>>>
>>>> The only documented SoC Hi3516DV300 does not receive any updates from 8
>>>> years ago. With the recent driver changes, it unlikely works for this
>>>> SoC anymore. Remove the binding for this SoC.
>>>>
>>>> Also it's hard to get the version number and it's unknown how the
>>>> version can be used. Remove them until it's really needed.
>>>>
>>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>>>> ---
>>>>  drivers/net/ethernet/hisilicon/hisi_femac.c | 4 +---
>>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
>>>> index eab91e011d11..9466ca9da2bb 100644
>>>> --- a/drivers/net/ethernet/hisilicon/hisi_femac.c
>>>> +++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
>>>> @@ -990,9 +990,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev)
>>>>  #endif
>>>>  
>>>>  static const struct of_device_id hisi_femac_match[] = {
>>>> -	{.compatible = "hisilicon,hisi-femac-v1",},
>>>> -	{.compatible = "hisilicon,hisi-femac-v2",},
>>>> -	{.compatible = "hisilicon,hi3516cv300-femac",},
>>>> +	{.compatible = "hisilicon,hisi-femac",},
>>>
>>> What is happening here? Removal could be justified, but then order of
>>> your patches is totally wrong. But that hisi-femac is a no-go or provide
>>> proper rationale.
>>
>> I don't understand exactly... In dts, we commonly have a SoC specific
>> compatible string first, generic compatible string the second. e.g.
>>
>> compatible = "hisilicon,hi3798mv200-perictrl", "syscon", "simple-mfd".
> 
> There is no generic compatible here. hi3798mv200 is soc.
> 
>>
>> So i think this is recommended. Or does it mean we only need them in
> 
> It is allowed for certain cases and recommended for even fewer ones. Do
> you want to say you have fully discoverable features here and you do not
> need any properties? Or you want to say that all possible hardware will
> have exactly the same programming interface (registers etc)?

Of course not. Take FEMAC for example. The FEMAC core on Hi3516 and
Hi3798 can programmed in the same way. They use the same
resources(resets and clocks). Though still a bit different in hardware,
basically the fifo depth etc..

Hi3516 FEMAC is not special enough to have a new compatible string, nor
do hi3798 FEMAC. Nor do i think we need those undocumented version
numbers, i.e. "hisilicon,hisi-femac-v1/2". As i observed, this driver is
generic enough to handle all the FEMAC cores i know, no matter which
version nor which SoC. This can also be concluded from the driver, the
driver defined 3 compatibles but they are all treated the same.

Should I remove all of them, and only leave a generic
"hisilicon,hisi-femac" instead?
> 
> Best regards,
> Krzysztof
> 

-- 
Best regards,
Yang Xiwen

Re: [PATCH net-next v4 6/6] net: hisi_femac: remove unused compatible strings

[Krzysztof Kozlowski]

On 27/02/2024 08:36, Yang Xiwen wrote:
> On 2/27/2024 2:53 PM, Krzysztof Kozlowski wrote:
>> On 27/02/2024 02:51, Yang Xiwen wrote:
>>> On 2/26/2024 3:55 PM, Krzysztof Kozlowski wrote:
>>>> On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote:
>>>>> From: Yang Xiwen <forbidden405@outlook.com>
>>>>>
>>>>> The only documented SoC Hi3516DV300 does not receive any updates from 8
>>>>> years ago. With the recent driver changes, it unlikely works for this
>>>>> SoC anymore. Remove the binding for this SoC.
>>>>>
>>>>> Also it's hard to get the version number and it's unknown how the
>>>>> version can be used. Remove them until it's really needed.
>>>>>
>>>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>>>>> ---
>>>>>  drivers/net/ethernet/hisilicon/hisi_femac.c | 4 +---
>>>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
>>>>> index eab91e011d11..9466ca9da2bb 100644
>>>>> --- a/drivers/net/ethernet/hisilicon/hisi_femac.c
>>>>> +++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
>>>>> @@ -990,9 +990,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev)
>>>>>  #endif
>>>>>  
>>>>>  static const struct of_device_id hisi_femac_match[] = {
>>>>> -	{.compatible = "hisilicon,hisi-femac-v1",},
>>>>> -	{.compatible = "hisilicon,hisi-femac-v2",},
>>>>> -	{.compatible = "hisilicon,hi3516cv300-femac",},
>>>>> +	{.compatible = "hisilicon,hisi-femac",},
>>>>
>>>> What is happening here? Removal could be justified, but then order of
>>>> your patches is totally wrong. But that hisi-femac is a no-go or provide
>>>> proper rationale.
>>>
>>> I don't understand exactly... In dts, we commonly have a SoC specific
>>> compatible string first, generic compatible string the second. e.g.
>>>
>>> compatible = "hisilicon,hi3798mv200-perictrl", "syscon", "simple-mfd".
>>
>> There is no generic compatible here. hi3798mv200 is soc.
>>
>>>
>>> So i think this is recommended. Or does it mean we only need them in
>>
>> It is allowed for certain cases and recommended for even fewer ones. Do
>> you want to say you have fully discoverable features here and you do not
>> need any properties? Or you want to say that all possible hardware will
>> have exactly the same programming interface (registers etc)?
> 
> Of course not. Take FEMAC for example. The FEMAC core on Hi3516 and

If they have different programming interface then you cannot use generic
compatible as fallback.

> Hi3798 can programmed in the same way. They use the same
> resources(resets and clocks). Though still a bit different in hardware,
> basically the fifo depth etc..
> 
> Hi3516 FEMAC is not special enough to have a new compatible string, nor
> do hi3798 FEMAC. Nor do i think we need those undocumented version
> numbers, i.e. "hisilicon,hisi-femac-v1/2". As i observed, this driver is
> generic enough to handle all the FEMAC cores i know, no matter which
> version nor which SoC. This can also be concluded from the driver, the
> driver defined 3 compatibles but they are all treated the same.
> 
> Should I remove all of them, and only leave a generic
> "hisilicon,hisi-femac" instead?

No.

Use one SoC specific compatible as fallback. That's what we ask almost
every time...

Best regards,
Krzysztof