Re: [PATCH 2/3] arm64: dts: imx8ulp: Add the fec support

Re: [PATCH 2/3] arm64: dts: imx8ulp: Add the fec support

[Ahmad Fatoum]

Hello Wei,

On 04.07.22 12:10, Wei Fang wrote:
> +	clock_ext_rmii: clock-ext-rmii {
> +		compatible = "fixed-clock";
> +		clock-frequency = <50000000>;
> +		#clock-cells = <0>;
> +		clock-output-names = "ext_rmii_clk";
> +	};
> +
> +	clock_ext_ts: clock-ext-ts {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-output-names = "ext_ts_clk";
> +	};

How are these SoC-specific? They sound like they belong
into board DT.

> +				clocks = <&cgc1 IMX8ULP_CLK_XBAR_DIVBUS>,
> +					 <&pcc4 IMX8ULP_CLK_ENET>,
> +					 <&cgc1 IMX8ULP_CLK_ENET_TS_SEL>,
> +					 <&clock_ext_rmii>;
> +				clock-names = "ipg", "ahb", "ptp", "enet_clk_ref";

I think the default should be the other way round, assume MAC to provide reference
clock and allow override on board-level if PHY does it instead.

Cheers,
Ahmad


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/3] arm64: dts: imx8ulp: Add the fec support

[Andrew Lunn]

> > +				clocks = <&cgc1 IMX8ULP_CLK_XBAR_DIVBUS>,
> > +					 <&pcc4 IMX8ULP_CLK_ENET>,
> > +					 <&cgc1 IMX8ULP_CLK_ENET_TS_SEL>,
> > +					 <&clock_ext_rmii>;
> > +				clock-names = "ipg", "ahb", "ptp", "enet_clk_ref";
> 
> I think the default should be the other way round, assume MAC to provide reference
> clock and allow override on board-level if PHY does it instead.

I would make it the same as all the other instances of FEC in the
IMX7, IMX6, IMX5, IMX4, Vybrid etc

      Andrew

Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Krzysztof Kozlowski]

On 04/07/2022 12:10, Wei Fang wrote:
> Add compatible item for i.MX8ULP platform.

Wrong subject prefix (dt-bindings).

Wrong subject contents - do not use some generic sentences like "update
X", just write what you are doing or what you want to achieve. For example:
dt-bindings: net: fsl,fec: add i.MX8 ULP FEC

> 
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  Documentation/devicetree/bindings/net/fsl,fec.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/fsl,fec.yaml b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> index daa2f79a294f..6642c246951b 100644
> --- a/Documentation/devicetree/bindings/net/fsl,fec.yaml
> +++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> @@ -40,6 +40,10 @@ properties:
>            - enum:
>                - fsl,imx7d-fec
>            - const: fsl,imx6sx-fec
> +      - items:
> +          - enum:
> +              - fsl,imx8ulp-fec
> +          - const: fsl,imx6ul-fec

This is wrong.  fsl,imx6ul-fec has to be followed by fsl,imx6q-fec. I
think someone made similar mistakes earlier so this is a mess.

>        - items:
>            - const: fsl,imx8mq-fec
>            - const: fsl,imx6sx-fec


Best regards,
Krzysztof

[PATCH 0/3] Add the fec node on i.MX8ULP platform

[Wei Fang]

Add the fec node for i.MX8ULP  platform.
And enable the fec support on i.MX8ULP EVK board.

Wei Fang (3):
  dt-bings: net: fsl,fec: update compatible item
  arm64: dts: imx8ulp: Add the fec support
  arm64: dts: imx8ulp-evk: Add the fec support

 .../devicetree/bindings/net/fsl,fec.yaml      |  4 ++
 arch/arm64/boot/dts/freescale/imx8ulp-evk.dts | 42 +++++++++++++++++++
 arch/arm64/boot/dts/freescale/imx8ulp.dtsi    | 29 +++++++++++++
 3 files changed, 75 insertions(+)

-- 
2.25.1

[PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Wei Fang]

Add compatible item for i.MX8ULP platform.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 Documentation/devicetree/bindings/net/fsl,fec.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/fsl,fec.yaml b/Documentation/devicetree/bindings/net/fsl,fec.yaml
index daa2f79a294f..6642c246951b 100644
--- a/Documentation/devicetree/bindings/net/fsl,fec.yaml
+++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml
@@ -40,6 +40,10 @@ properties:
           - enum:
               - fsl,imx7d-fec
           - const: fsl,imx6sx-fec
+      - items:
+          - enum:
+              - fsl,imx8ulp-fec
+          - const: fsl,imx6ul-fec
       - items:
           - const: fsl,imx8mq-fec
           - const: fsl,imx6sx-fec
-- 
2.25.1

[PATCH 2/3] arm64: dts: imx8ulp: Add the fec support

[Wei Fang]

Add the fec support on i.MX8ULP platforms.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8ulp.dtsi | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8ulp.dtsi b/arch/arm64/boot/dts/freescale/imx8ulp.dtsi
index 60c1b018bf03..822f3aea46e1 100644
--- a/arch/arm64/boot/dts/freescale/imx8ulp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8ulp.dtsi
@@ -16,6 +16,7 @@ / {
 	#size-cells = <2>;
 
 	aliases {
+		ethernet0 = &fec;
 		gpio0 = &gpiod;
 		gpio1 = &gpioe;
 		gpio2 = &gpiof;
@@ -137,6 +138,19 @@ scmi_sensor: protocol@15 {
 		};
 	};
 
+	clock_ext_rmii: clock-ext-rmii {
+		compatible = "fixed-clock";
+		clock-frequency = <50000000>;
+		#clock-cells = <0>;
+		clock-output-names = "ext_rmii_clk";
+	};
+
+	clock_ext_ts: clock-ext-ts {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-output-names = "ext_ts_clk";
+	};
+
 	soc: soc@0 {
 		compatible = "simple-bus";
 		#address-cells = <1>;
@@ -365,6 +379,21 @@ usdhc2: mmc@298f0000 {
 				bus-width = <4>;
 				status = "disabled";
 			};
+
+			fec: ethernet@29950000 {
+				compatible = "fsl,imx8ulp-fec", "fsl,imx6ul-fec";
+				reg = <0x29950000 0x10000>;
+				interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-names = "int0";
+				clocks = <&cgc1 IMX8ULP_CLK_XBAR_DIVBUS>,
+					 <&pcc4 IMX8ULP_CLK_ENET>,
+					 <&cgc1 IMX8ULP_CLK_ENET_TS_SEL>,
+					 <&clock_ext_rmii>;
+				clock-names = "ipg", "ahb", "ptp", "enet_clk_ref";
+				fsl,num-tx-queues = <1>;
+				fsl,num-rx-queues = <1>;
+				status = "disabled";
+			};
 		};
 
 		gpioe: gpio@2d000080 {
-- 
2.25.1

[PATCH 3/3] arm64: dts: imx8ulp-evk: Add the fec support

[Wei Fang]

Enable the fec on i.MX8ULP EVK board.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8ulp-evk.dts | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8ulp-evk.dts b/arch/arm64/boot/dts/freescale/imx8ulp-evk.dts
index 33e84c4e9ed8..ac635022ab45 100644
--- a/arch/arm64/boot/dts/freescale/imx8ulp-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8ulp-evk.dts
@@ -38,7 +38,49 @@ &usdhc0 {
 	status = "okay";
 };
 
+&clock_ext_ts {
+	/* External ts clock is 50MHZ from PHY on EVK board. */
+	clock-frequency = <50000000>;
+};
+
+&fec {
+	pinctrl-names = "default", "sleep";
+	pinctrl-0 = <&pinctrl_enet>;
+	pinctrl-1 = <&pinctrl_enet>;
+	assigned-clocks = <&cgc1 IMX8ULP_CLK_ENET_TS_SEL>;
+	assigned-clock-parents = <&clock_ext_ts>;
+	phy-mode = "rmii";
+	phy-handle = <&ethphy>;
+	status = "okay";
+
+	mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		ethphy: ethernet-phy {
+			reg = <1>;
+			micrel,led-mode = <1>;
+		};
+	};
+};
+
 &iomuxc1 {
+	pinctrl_enet: enetgrp {
+		fsl,pins = <
+			MX8ULP_PAD_PTE15__ENET0_MDC     0x43
+			MX8ULP_PAD_PTE14__ENET0_MDIO    0x43
+			MX8ULP_PAD_PTE17__ENET0_RXER    0x43
+			MX8ULP_PAD_PTE18__ENET0_CRS_DV  0x43
+			MX8ULP_PAD_PTF1__ENET0_RXD0     0x43
+			MX8ULP_PAD_PTE20__ENET0_RXD1    0x43
+			MX8ULP_PAD_PTE16__ENET0_TXEN    0x43
+			MX8ULP_PAD_PTE23__ENET0_TXD0    0x43
+			MX8ULP_PAD_PTE22__ENET0_TXD1    0x43
+			MX8ULP_PAD_PTE19__ENET0_REFCLK  0x43
+			MX8ULP_PAD_PTF10__ENET0_1588_CLKIN 0x43
+		>;
+	};
+
 	pinctrl_lpuart5: lpuart5grp {
 		fsl,pins = <
 			MX8ULP_PAD_PTF14__LPUART5_TX	0x3
-- 
2.25.1

RE: [EXT] Re: [PATCH 2/3] arm64: dts: imx8ulp: Add the fec support

[Wei Fang]

Hi Ahmad:

	Thanks for your reply. clock_ext_rmii and clock_ext_ts indeed belong into board DT, I will move them to imx.8ulp-evk.dts and resubmit the patch. And refer to imx8ulp reference manual, the enet_clk_ref only has external clock source, so it is related to specifical board. Therefore, can I delete the enet_clk_ref clock in imx8ulp.dtsi (as shown below) and override the clock and clock-names properties in imx8ulp-evk.dts ?

> +                             clocks = <&cgc1 IMX8ULP_CLK_XBAR_DIVBUS>,
> +                                      <&pcc4 IMX8ULP_CLK_ENET>,
> +                                      <&cgc1 IMX8ULP_CLK_ENET_TS_SEL>;
> +                             clock-names = "ipg", "ahb", "ptp";


-----Original Message-----
From: Ahmad Fatoum <a.fatoum@pengutronix.de> 
Sent: 2022年7月4日 15:07
To: Wei Fang <wei.fang@nxp.com>; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org; s.hauer@pengutronix.de
Cc: Aisheng Dong <aisheng.dong@nxp.com>; devicetree@vger.kernel.org; Peng Fan <peng.fan@nxp.com>; Jacky Bai <ping.bai@nxp.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; sudeep.holla@arm.com; festevam@gmail.com; linux-arm-kernel@lists.infradead.org
Subject: [EXT] Re: [PATCH 2/3] arm64: dts: imx8ulp: Add the fec support

Caution: EXT Email

Hello Wei,

On 04.07.22 12:10, Wei Fang wrote:
> +     clock_ext_rmii: clock-ext-rmii {
> +             compatible = "fixed-clock";
> +             clock-frequency = <50000000>;
> +             #clock-cells = <0>;
> +             clock-output-names = "ext_rmii_clk";
> +     };
> +
> +     clock_ext_ts: clock-ext-ts {
> +             compatible = "fixed-clock";
> +             #clock-cells = <0>;
> +             clock-output-names = "ext_ts_clk";
> +     };

How are these SoC-specific? They sound like they belong into board DT.

> +                             clocks = <&cgc1 IMX8ULP_CLK_XBAR_DIVBUS>,
> +                                      <&pcc4 IMX8ULP_CLK_ENET>,
> +                                      <&cgc1 IMX8ULP_CLK_ENET_TS_SEL>,
> +                                      <&clock_ext_rmii>;
> +                             clock-names = "ipg", "ahb", "ptp", 
> + "enet_clk_ref";

I think the default should be the other way round, assume MAC to provide reference clock and allow override on board-level if PHY does it instead.

Cheers,
Ahmad


--
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pengutronix.de%2F&amp;data=05%7C01%7Cwei.fang%40nxp.com%7C9dad0367d54b427c5e8008da5d8bd912%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637925152473535653%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=3tkqsToqq7%2BvNDkC7ZaMm0DsisugDpkVQXCr2zqPbF8%3D&amp;reserved=0  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [EXT] Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Wei Fang]

Hi Krzysztof,
	
	Sorry, I'm still a little confused. Do you mean to modify as follows?
> +      - items:
> +          - enum:
> +              - fsl,imx8ulp-fec
> +          - const: fsl,imx6ul-fec
> +          - const: fsl,imx6q-fec

And as far as I know, the imx8ulp's fec is reused from imx6ul, they both have the same features. However, the fec of imx8ulp(and imx6ul) is a little different from imx6q, therefore, the functions supported by the driver are also somewhat different. 

-----Original Message-----
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 
Sent: 2022年7月4日 17:12
To: Wei Fang <wei.fang@nxp.com>; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org; s.hauer@pengutronix.de
Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; Jacky Bai <ping.bai@nxp.com>; sudeep.holla@arm.com; linux-arm-kernel@lists.infradead.org; Aisheng Dong <aisheng.dong@nxp.com>
Subject: [EXT] Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

Caution: EXT Email

On 04/07/2022 12:10, Wei Fang wrote:
> Add compatible item for i.MX8ULP platform.

Wrong subject prefix (dt-bindings).

Wrong subject contents - do not use some generic sentences like "update X", just write what you are doing or what you want to achieve. For example:
dt-bindings: net: fsl,fec: add i.MX8 ULP FEC

>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  Documentation/devicetree/bindings/net/fsl,fec.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/fsl,fec.yaml 
> b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> index daa2f79a294f..6642c246951b 100644
> --- a/Documentation/devicetree/bindings/net/fsl,fec.yaml
> +++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> @@ -40,6 +40,10 @@ properties:
>            - enum:
>                - fsl,imx7d-fec
>            - const: fsl,imx6sx-fec
> +      - items:
> +          - enum:
> +              - fsl,imx8ulp-fec
> +          - const: fsl,imx6ul-fec

This is wrong.  fsl,imx6ul-fec has to be followed by fsl,imx6q-fec. I think someone made similar mistakes earlier so this is a mess.

>        - items:
>            - const: fsl,imx8mq-fec
>            - const: fsl,imx6sx-fec


Best regards,
Krzysztof

Re: [EXT] Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Krzysztof Kozlowski]

On 05/07/2022 04:47, Wei Fang wrote:
> Hi Krzysztof,
> 	
> 	Sorry, I'm still a little confused. Do you mean to modify as follows?
>> +      - items:
>> +          - enum:
>> +              - fsl,imx8ulp-fec
>> +          - const: fsl,imx6ul-fec
>> +          - const: fsl,imx6q-fec

Yes

> 
> And as far as I know, the imx8ulp's fec is reused from imx6ul, they both have the same features. However, the fec of imx8ulp(and imx6ul) is a little different from imx6q, therefore, the functions supported by the driver are also somewhat different. 

I understand. But if imx8ulp is the same as imx6ul and imx6ul is
compatible with imx6q, then I expect imx8ulp to be compatible with
imx6ul and with imx6q.

Best regards,
Krzysztof

RE: [EXT] Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Wei Fang]

Hi Krzysztof,

	Thanks for your suggestion, I will resubmit the patch.

-----Original Message-----
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 
Sent: 2022年7月5日 15:27
To: Wei Fang <wei.fang@nxp.com>; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org; s.hauer@pengutronix.de
Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; Jacky Bai <ping.bai@nxp.com>; sudeep.holla@arm.com; linux-arm-kernel@lists.infradead.org; Aisheng Dong <aisheng.dong@nxp.com>
Subject: Re: [EXT] Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

Caution: EXT Email

On 05/07/2022 04:47, Wei Fang wrote:
> Hi Krzysztof,
>
>       Sorry, I'm still a little confused. Do you mean to modify as follows?
>> +      - items:
>> +          - enum:
>> +              - fsl,imx8ulp-fec
>> +          - const: fsl,imx6ul-fec
>> +          - const: fsl,imx6q-fec

Yes

>
> And as far as I know, the imx8ulp's fec is reused from imx6ul, they both have the same features. However, the fec of imx8ulp(and imx6ul) is a little different from imx6q, therefore, the functions supported by the driver are also somewhat different.

I understand. But if imx8ulp is the same as imx6ul and imx6ul is compatible with imx6q, then I expect imx8ulp to be compatible with imx6ul and with imx6q.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Shawn Guo]

On Mon, Jul 04, 2022 at 11:12:09AM +0200, Krzysztof Kozlowski wrote:
> > diff --git a/Documentation/devicetree/bindings/net/fsl,fec.yaml b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> > index daa2f79a294f..6642c246951b 100644
> > --- a/Documentation/devicetree/bindings/net/fsl,fec.yaml
> > +++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> > @@ -40,6 +40,10 @@ properties:
> >            - enum:
> >                - fsl,imx7d-fec
> >            - const: fsl,imx6sx-fec
> > +      - items:
> > +          - enum:
> > +              - fsl,imx8ulp-fec
> > +          - const: fsl,imx6ul-fec
> 
> This is wrong.  fsl,imx6ul-fec has to be followed by fsl,imx6q-fec. I
> think someone made similar mistakes earlier so this is a mess.

Hmm, not sure I follow this.  Supposing we want to have the following
compatible for i.MX8ULP FEC, why do we have to have "fsl,imx6q-fec"
here?

	fec: ethernet@29950000 {
		compatible = "fsl,imx8ulp-fec", "fsl,imx6ul-fec";
		...
	};

Shawn

> 
> >        - items:
> >            - const: fsl,imx8mq-fec
> >            - const: fsl,imx6sx-fec

Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Krzysztof Kozlowski]

On 18/08/2022 04:33, Shawn Guo wrote:
> On Mon, Jul 04, 2022 at 11:12:09AM +0200, Krzysztof Kozlowski wrote:
>>> diff --git a/Documentation/devicetree/bindings/net/fsl,fec.yaml b/Documentation/devicetree/bindings/net/fsl,fec.yaml
>>> index daa2f79a294f..6642c246951b 100644
>>> --- a/Documentation/devicetree/bindings/net/fsl,fec.yaml
>>> +++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml
>>> @@ -40,6 +40,10 @@ properties:
>>>            - enum:
>>>                - fsl,imx7d-fec
>>>            - const: fsl,imx6sx-fec
>>> +      - items:
>>> +          - enum:
>>> +              - fsl,imx8ulp-fec
>>> +          - const: fsl,imx6ul-fec
>>
>> This is wrong.  fsl,imx6ul-fec has to be followed by fsl,imx6q-fec. I
>> think someone made similar mistakes earlier so this is a mess.
> 
> Hmm, not sure I follow this.  Supposing we want to have the following
> compatible for i.MX8ULP FEC, why do we have to have "fsl,imx6q-fec"
> here?
> 
> 	fec: ethernet@29950000 {
> 		compatible = "fsl,imx8ulp-fec", "fsl,imx6ul-fec";
> 		...
> 	};

Because a bit earlier this bindings is saying that fsl,imx6ul-fec must
be followed by fsl,imx6q-fec.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Shawn Guo]

On Thu, Aug 18, 2022 at 10:51:02AM +0300, Krzysztof Kozlowski wrote:
> On 18/08/2022 04:33, Shawn Guo wrote:
> > On Mon, Jul 04, 2022 at 11:12:09AM +0200, Krzysztof Kozlowski wrote:
> >>> diff --git a/Documentation/devicetree/bindings/net/fsl,fec.yaml b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> >>> index daa2f79a294f..6642c246951b 100644
> >>> --- a/Documentation/devicetree/bindings/net/fsl,fec.yaml
> >>> +++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> >>> @@ -40,6 +40,10 @@ properties:
> >>>            - enum:
> >>>                - fsl,imx7d-fec
> >>>            - const: fsl,imx6sx-fec
> >>> +      - items:
> >>> +          - enum:
> >>> +              - fsl,imx8ulp-fec
> >>> +          - const: fsl,imx6ul-fec
> >>
> >> This is wrong.  fsl,imx6ul-fec has to be followed by fsl,imx6q-fec. I
> >> think someone made similar mistakes earlier so this is a mess.
> > 
> > Hmm, not sure I follow this.  Supposing we want to have the following
> > compatible for i.MX8ULP FEC, why do we have to have "fsl,imx6q-fec"
> > here?
> > 
> > 	fec: ethernet@29950000 {
> > 		compatible = "fsl,imx8ulp-fec", "fsl,imx6ul-fec";
> > 		...
> > 	};
> 
> Because a bit earlier this bindings is saying that fsl,imx6ul-fec must
> be followed by fsl,imx6q-fec.

The FEC driver OF match table suggests that fsl,imx6ul-fec and fsl,imx6q-fec
are not really compatible.

static const struct of_device_id fec_dt_ids[] = {
        { .compatible = "fsl,imx25-fec", .data = &fec_devtype[IMX25_FEC], },
        { .compatible = "fsl,imx27-fec", .data = &fec_devtype[IMX27_FEC], },
        { .compatible = "fsl,imx28-fec", .data = &fec_devtype[IMX28_FEC], },
        { .compatible = "fsl,imx6q-fec", .data = &fec_devtype[IMX6Q_FEC], },
        { .compatible = "fsl,mvf600-fec", .data = &fec_devtype[MVF600_FEC], },
        { .compatible = "fsl,imx6sx-fec", .data = &fec_devtype[IMX6SX_FEC], },
        { .compatible = "fsl,imx6ul-fec", .data = &fec_devtype[IMX6UL_FEC], },
        { .compatible = "fsl,imx8mq-fec", .data = &fec_devtype[IMX8MQ_FEC], },
        { .compatible = "fsl,imx8qm-fec", .data = &fec_devtype[IMX8QM_FEC], },
        { /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, fec_dt_ids);

Should we fix the binding doc?

Shawn

Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Krzysztof Kozlowski]

On 18/08/2022 12:22, Shawn Guo wrote:
> On Thu, Aug 18, 2022 at 10:51:02AM +0300, Krzysztof Kozlowski wrote:
>> On 18/08/2022 04:33, Shawn Guo wrote:
>>> On Mon, Jul 04, 2022 at 11:12:09AM +0200, Krzysztof Kozlowski wrote:
>>>>> diff --git a/Documentation/devicetree/bindings/net/fsl,fec.yaml b/Documentation/devicetree/bindings/net/fsl,fec.yaml
>>>>> index daa2f79a294f..6642c246951b 100644
>>>>> --- a/Documentation/devicetree/bindings/net/fsl,fec.yaml
>>>>> +++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml
>>>>> @@ -40,6 +40,10 @@ properties:
>>>>>            - enum:
>>>>>                - fsl,imx7d-fec
>>>>>            - const: fsl,imx6sx-fec
>>>>> +      - items:
>>>>> +          - enum:
>>>>> +              - fsl,imx8ulp-fec
>>>>> +          - const: fsl,imx6ul-fec
>>>>
>>>> This is wrong.  fsl,imx6ul-fec has to be followed by fsl,imx6q-fec. I
>>>> think someone made similar mistakes earlier so this is a mess.
>>>
>>> Hmm, not sure I follow this.  Supposing we want to have the following
>>> compatible for i.MX8ULP FEC, why do we have to have "fsl,imx6q-fec"
>>> here?
>>>
>>> 	fec: ethernet@29950000 {
>>> 		compatible = "fsl,imx8ulp-fec", "fsl,imx6ul-fec";
>>> 		...
>>> 	};
>>
>> Because a bit earlier this bindings is saying that fsl,imx6ul-fec must
>> be followed by fsl,imx6q-fec.
> 
> The FEC driver OF match table suggests that fsl,imx6ul-fec and fsl,imx6q-fec
> are not really compatible.
> 
> static const struct of_device_id fec_dt_ids[] = {
>         { .compatible = "fsl,imx25-fec", .data = &fec_devtype[IMX25_FEC], },
>         { .compatible = "fsl,imx27-fec", .data = &fec_devtype[IMX27_FEC], },
>         { .compatible = "fsl,imx28-fec", .data = &fec_devtype[IMX28_FEC], },
>         { .compatible = "fsl,imx6q-fec", .data = &fec_devtype[IMX6Q_FEC], },
>         { .compatible = "fsl,mvf600-fec", .data = &fec_devtype[MVF600_FEC], },
>         { .compatible = "fsl,imx6sx-fec", .data = &fec_devtype[IMX6SX_FEC], },
>         { .compatible = "fsl,imx6ul-fec", .data = &fec_devtype[IMX6UL_FEC], },

I don't see here any incompatibility. Binding driver with different
driver data is not a proof of incompatible devices. Additionally, the
binding describes the hardware, not the driver.

>         { .compatible = "fsl,imx8mq-fec", .data = &fec_devtype[IMX8MQ_FEC], },
>         { .compatible = "fsl,imx8qm-fec", .data = &fec_devtype[IMX8QM_FEC], },
>         { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, fec_dt_ids);
> 
> Should we fix the binding doc?

Maybe, I don't know. The binding describes the hardware, so based on it
the devices are compatible. Changing this, except ABI impact, would be
possible with proper reason, but not based on Linux driver code.


Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Shawn Guo]

On Thu, Aug 18, 2022 at 12:46:33PM +0300, Krzysztof Kozlowski wrote:
> On 18/08/2022 12:22, Shawn Guo wrote:
> > On Thu, Aug 18, 2022 at 10:51:02AM +0300, Krzysztof Kozlowski wrote:
> >> On 18/08/2022 04:33, Shawn Guo wrote:
> >>> On Mon, Jul 04, 2022 at 11:12:09AM +0200, Krzysztof Kozlowski wrote:
> >>>>> diff --git a/Documentation/devicetree/bindings/net/fsl,fec.yaml b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> >>>>> index daa2f79a294f..6642c246951b 100644
> >>>>> --- a/Documentation/devicetree/bindings/net/fsl,fec.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> >>>>> @@ -40,6 +40,10 @@ properties:
> >>>>>            - enum:
> >>>>>                - fsl,imx7d-fec
> >>>>>            - const: fsl,imx6sx-fec
> >>>>> +      - items:
> >>>>> +          - enum:
> >>>>> +              - fsl,imx8ulp-fec
> >>>>> +          - const: fsl,imx6ul-fec
> >>>>
> >>>> This is wrong.  fsl,imx6ul-fec has to be followed by fsl,imx6q-fec. I
> >>>> think someone made similar mistakes earlier so this is a mess.
> >>>
> >>> Hmm, not sure I follow this.  Supposing we want to have the following
> >>> compatible for i.MX8ULP FEC, why do we have to have "fsl,imx6q-fec"
> >>> here?
> >>>
> >>> 	fec: ethernet@29950000 {
> >>> 		compatible = "fsl,imx8ulp-fec", "fsl,imx6ul-fec";
> >>> 		...
> >>> 	};
> >>
> >> Because a bit earlier this bindings is saying that fsl,imx6ul-fec must
> >> be followed by fsl,imx6q-fec.
> > 
> > The FEC driver OF match table suggests that fsl,imx6ul-fec and fsl,imx6q-fec
> > are not really compatible.
> > 
> > static const struct of_device_id fec_dt_ids[] = {
> >         { .compatible = "fsl,imx25-fec", .data = &fec_devtype[IMX25_FEC], },
> >         { .compatible = "fsl,imx27-fec", .data = &fec_devtype[IMX27_FEC], },
> >         { .compatible = "fsl,imx28-fec", .data = &fec_devtype[IMX28_FEC], },
> >         { .compatible = "fsl,imx6q-fec", .data = &fec_devtype[IMX6Q_FEC], },
> >         { .compatible = "fsl,mvf600-fec", .data = &fec_devtype[MVF600_FEC], },
> >         { .compatible = "fsl,imx6sx-fec", .data = &fec_devtype[IMX6SX_FEC], },
> >         { .compatible = "fsl,imx6ul-fec", .data = &fec_devtype[IMX6UL_FEC], },
> 
> I don't see here any incompatibility. Binding driver with different
> driver data is not a proof of incompatible devices.

To me, different driver data is a good sign of incompatibility.  It
mostly means that software needs to program the hardware block
differently.


> Additionally, the
> binding describes the hardware, not the driver.
> 
> >         { .compatible = "fsl,imx8mq-fec", .data = &fec_devtype[IMX8MQ_FEC], },
> >         { .compatible = "fsl,imx8qm-fec", .data = &fec_devtype[IMX8QM_FEC], },
> >         { /* sentinel */ }
> > };
> > MODULE_DEVICE_TABLE(of, fec_dt_ids);
> > 
> > Should we fix the binding doc?
> 
> Maybe, I don't know. The binding describes the hardware, so based on it
> the devices are compatible. Changing this, except ABI impact, would be
> possible with proper reason, but not based on Linux driver code.

Well, if Linux driver code is written in the way that hardware requires,
I guess that's just based on hardware characteristics.

To me, having a device compatible to two devices that require different
programming model is unnecessary and confusing.

Shawn

Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Krzysztof Kozlowski]

On 18/08/2022 16:57, Shawn Guo wrote:
> On Thu, Aug 18, 2022 at 12:46:33PM +0300, Krzysztof Kozlowski wrote:
>> On 18/08/2022 12:22, Shawn Guo wrote:
>>> On Thu, Aug 18, 2022 at 10:51:02AM +0300, Krzysztof Kozlowski wrote:
>>>> On 18/08/2022 04:33, Shawn Guo wrote:
>>>>> On Mon, Jul 04, 2022 at 11:12:09AM +0200, Krzysztof Kozlowski wrote:
>>>>>>> diff --git a/Documentation/devicetree/bindings/net/fsl,fec.yaml b/Documentation/devicetree/bindings/net/fsl,fec.yaml
>>>>>>> index daa2f79a294f..6642c246951b 100644
>>>>>>> --- a/Documentation/devicetree/bindings/net/fsl,fec.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml
>>>>>>> @@ -40,6 +40,10 @@ properties:
>>>>>>>            - enum:
>>>>>>>                - fsl,imx7d-fec
>>>>>>>            - const: fsl,imx6sx-fec
>>>>>>> +      - items:
>>>>>>> +          - enum:
>>>>>>> +              - fsl,imx8ulp-fec
>>>>>>> +          - const: fsl,imx6ul-fec
>>>>>>
>>>>>> This is wrong.  fsl,imx6ul-fec has to be followed by fsl,imx6q-fec. I
>>>>>> think someone made similar mistakes earlier so this is a mess.
>>>>>
>>>>> Hmm, not sure I follow this.  Supposing we want to have the following
>>>>> compatible for i.MX8ULP FEC, why do we have to have "fsl,imx6q-fec"
>>>>> here?
>>>>>
>>>>> 	fec: ethernet@29950000 {
>>>>> 		compatible = "fsl,imx8ulp-fec", "fsl,imx6ul-fec";
>>>>> 		...
>>>>> 	};
>>>>
>>>> Because a bit earlier this bindings is saying that fsl,imx6ul-fec must
>>>> be followed by fsl,imx6q-fec.
>>>
>>> The FEC driver OF match table suggests that fsl,imx6ul-fec and fsl,imx6q-fec
>>> are not really compatible.
>>>
>>> static const struct of_device_id fec_dt_ids[] = {
>>>         { .compatible = "fsl,imx25-fec", .data = &fec_devtype[IMX25_FEC], },
>>>         { .compatible = "fsl,imx27-fec", .data = &fec_devtype[IMX27_FEC], },
>>>         { .compatible = "fsl,imx28-fec", .data = &fec_devtype[IMX28_FEC], },
>>>         { .compatible = "fsl,imx6q-fec", .data = &fec_devtype[IMX6Q_FEC], },
>>>         { .compatible = "fsl,mvf600-fec", .data = &fec_devtype[MVF600_FEC], },
>>>         { .compatible = "fsl,imx6sx-fec", .data = &fec_devtype[IMX6SX_FEC], },
>>>         { .compatible = "fsl,imx6ul-fec", .data = &fec_devtype[IMX6UL_FEC], },
>>
>> I don't see here any incompatibility. Binding driver with different
>> driver data is not a proof of incompatible devices.
> 
> To me, different driver data is a good sign of incompatibility.  It
> mostly means that software needs to program the hardware block
> differently.

Any device being 100% compatible with old one and having additional
features will have different driver data... so no, it's not a proof.
There are many of such examples and we call them compatible, because the
device could operate when bound by the fallback compatible.

If this is the case here - how do I know? I raised and the answer was
affirmative...

> 
> 
>> Additionally, the
>> binding describes the hardware, not the driver.
>>
>>>         { .compatible = "fsl,imx8mq-fec", .data = &fec_devtype[IMX8MQ_FEC], },
>>>         { .compatible = "fsl,imx8qm-fec", .data = &fec_devtype[IMX8QM_FEC], },
>>>         { /* sentinel */ }
>>> };
>>> MODULE_DEVICE_TABLE(of, fec_dt_ids);
>>>
>>> Should we fix the binding doc?
>>
>> Maybe, I don't know. The binding describes the hardware, so based on it
>> the devices are compatible. Changing this, except ABI impact, would be
>> possible with proper reason, but not based on Linux driver code.
> 
> Well, if Linux driver code is written in the way that hardware requires,
> I guess that's just based on hardware characteristics.
> 
> To me, having a device compatible to two devices that require different
> programming model is unnecessary and confusing.

It's the first time anyone mentions here the programming model is
different... If it is different, the devices are likely not compatible.

However when I raised this issue last time, there were no concerns with
calling them all compatible. Therefore I wonder if the folks working on
this driver actually know what's there... I don't know, I gave
recommendations based on what is described in the binding and expect the
engineer to come with that knowledge.


Best regards,
Krzysztof

RE: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Wei Fang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: 2022年8月18日 22:04
> To: Shawn Guo <shawnguo@kernel.org>
> Cc: Wei Fang <wei.fang@nxp.com>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> s.hauer@pengutronix.de; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; festevam@gmail.com;
> dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; Jacky Bai
> <ping.bai@nxp.com>; sudeep.holla@arm.com;
> linux-arm-kernel@lists.infradead.org; Aisheng Dong <aisheng.dong@nxp.com>
> Subject: Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item
> 
> On 18/08/2022 16:57, Shawn Guo wrote:
> > On Thu, Aug 18, 2022 at 12:46:33PM +0300, Krzysztof Kozlowski wrote:
> >> On 18/08/2022 12:22, Shawn Guo wrote:
> >>> On Thu, Aug 18, 2022 at 10:51:02AM +0300, Krzysztof Kozlowski wrote:
> >>>> On 18/08/2022 04:33, Shawn Guo wrote:
> >>>>> On Mon, Jul 04, 2022 at 11:12:09AM +0200, Krzysztof Kozlowski wrote:
> >>>>>>> diff --git a/Documentation/devicetree/bindings/net/fsl,fec.yaml
> >>>>>>> b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> >>>>>>> index daa2f79a294f..6642c246951b 100644
> >>>>>>> --- a/Documentation/devicetree/bindings/net/fsl,fec.yaml
> >>>>>>> +++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> >>>>>>> @@ -40,6 +40,10 @@ properties:
> >>>>>>>            - enum:
> >>>>>>>                - fsl,imx7d-fec
> >>>>>>>            - const: fsl,imx6sx-fec
> >>>>>>> +      - items:
> >>>>>>> +          - enum:
> >>>>>>> +              - fsl,imx8ulp-fec
> >>>>>>> +          - const: fsl,imx6ul-fec
> >>>>>>
> >>>>>> This is wrong.  fsl,imx6ul-fec has to be followed by
> >>>>>> fsl,imx6q-fec. I think someone made similar mistakes earlier so this is a
> mess.
> >>>>>
> >>>>> Hmm, not sure I follow this.  Supposing we want to have the
> >>>>> following compatible for i.MX8ULP FEC, why do we have to have
> "fsl,imx6q-fec"
> >>>>> here?
> >>>>>
> >>>>> 	fec: ethernet@29950000 {
> >>>>> 		compatible = "fsl,imx8ulp-fec", "fsl,imx6ul-fec";
> >>>>> 		...
> >>>>> 	};
> >>>>
> >>>> Because a bit earlier this bindings is saying that fsl,imx6ul-fec
> >>>> must be followed by fsl,imx6q-fec.
> >>>
> >>> The FEC driver OF match table suggests that fsl,imx6ul-fec and
> >>> fsl,imx6q-fec are not really compatible.
> >>>
> >>> static const struct of_device_id fec_dt_ids[] = {
> >>>         { .compatible = "fsl,imx25-fec", .data =
> &fec_devtype[IMX25_FEC], },
> >>>         { .compatible = "fsl,imx27-fec", .data =
> &fec_devtype[IMX27_FEC], },
> >>>         { .compatible = "fsl,imx28-fec", .data =
> &fec_devtype[IMX28_FEC], },
> >>>         { .compatible = "fsl,imx6q-fec", .data =
> &fec_devtype[IMX6Q_FEC], },
> >>>         { .compatible = "fsl,mvf600-fec", .data =
> &fec_devtype[MVF600_FEC], },
> >>>         { .compatible = "fsl,imx6sx-fec", .data =
> &fec_devtype[IMX6SX_FEC], },
> >>>         { .compatible = "fsl,imx6ul-fec", .data =
> >>> &fec_devtype[IMX6UL_FEC], },
> >>
> >> I don't see here any incompatibility. Binding driver with different
> >> driver data is not a proof of incompatible devices.
> >
> > To me, different driver data is a good sign of incompatibility.  It
> > mostly means that software needs to program the hardware block
> > differently.
> 
> Any device being 100% compatible with old one and having additional features
> will have different driver data... so no, it's not a proof.
> There are many of such examples and we call them compatible, because the
> device could operate when bound by the fallback compatible.
> 
> If this is the case here - how do I know? I raised and the answer was
> affirmative...
> 
> >
> >
> >> Additionally, the
> >> binding describes the hardware, not the driver.
> >>
> >>>         { .compatible = "fsl,imx8mq-fec", .data =
> &fec_devtype[IMX8MQ_FEC], },
> >>>         { .compatible = "fsl,imx8qm-fec", .data =
> &fec_devtype[IMX8QM_FEC], },
> >>>         { /* sentinel */ }
> >>> };
> >>> MODULE_DEVICE_TABLE(of, fec_dt_ids);
> >>>
> >>> Should we fix the binding doc?
> >>
> >> Maybe, I don't know. The binding describes the hardware, so based on
> >> it the devices are compatible. Changing this, except ABI impact,
> >> would be possible with proper reason, but not based on Linux driver code.
> >
> > Well, if Linux driver code is written in the way that hardware
> > requires, I guess that's just based on hardware characteristics.
> >
> > To me, having a device compatible to two devices that require
> > different programming model is unnecessary and confusing.
> 
> It's the first time anyone mentions here the programming model is different... If
> it is different, the devices are likely not compatible.
> 
> However when I raised this issue last time, there were no concerns with calling
> them all compatible. Therefore I wonder if the folks working on this driver
> actually know what's there... I don't know, I gave recommendations based on
> what is described in the binding and expect the engineer to come with that
> knowledge.
> 
> 
Sorry, I did not explain clearly last time, I just mentioned that imx8ulp fec was
totally reused from imx6ul and was a little different from imx6q.
So what should I do next? Should I fix the binding doc ?

RE: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Peng Fan]

> Subject: RE: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item
> 
> 
> 
> > -----Original Message-----
> > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Sent: 2022年8月18日 22:04
> > To: Shawn Guo <shawnguo@kernel.org>
> > Cc: Wei Fang <wei.fang@nxp.com>; davem@davemloft.net;
> > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > s.hauer@pengutronix.de; netdev@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx
> > <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; Jacky Bai
> > <ping.bai@nxp.com>; sudeep.holla@arm.com;
> > linux-arm-kernel@lists.infradead.org; Aisheng Dong
> > <aisheng.dong@nxp.com>
> > Subject: Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible
> > item
> >
> > On 18/08/2022 16:57, Shawn Guo wrote:
> > > On Thu, Aug 18, 2022 at 12:46:33PM +0300, Krzysztof Kozlowski wrote:
> > >> On 18/08/2022 12:22, Shawn Guo wrote:
> > >>> On Thu, Aug 18, 2022 at 10:51:02AM +0300, Krzysztof Kozlowski wrote:
> > >>>> On 18/08/2022 04:33, Shawn Guo wrote:
> > >>>>> On Mon, Jul 04, 2022 at 11:12:09AM +0200, Krzysztof Kozlowski
> wrote:
> > >>>>>>> diff --git
> > >>>>>>> a/Documentation/devicetree/bindings/net/fsl,fec.yaml
> > >>>>>>> b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> > >>>>>>> index daa2f79a294f..6642c246951b 100644
> > >>>>>>> --- a/Documentation/devicetree/bindings/net/fsl,fec.yaml
> > >>>>>>> +++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> > >>>>>>> @@ -40,6 +40,10 @@ properties:
> > >>>>>>>            - enum:
> > >>>>>>>                - fsl,imx7d-fec
> > >>>>>>>            - const: fsl,imx6sx-fec
> > >>>>>>> +      - items:
> > >>>>>>> +          - enum:
> > >>>>>>> +              - fsl,imx8ulp-fec
> > >>>>>>> +          - const: fsl,imx6ul-fec
> > >>>>>>
> > >>>>>> This is wrong.  fsl,imx6ul-fec has to be followed by
> > >>>>>> fsl,imx6q-fec. I think someone made similar mistakes earlier so
> > >>>>>> this is a
> > mess.
> > >>>>>
> > >>>>> Hmm, not sure I follow this.  Supposing we want to have the
> > >>>>> following compatible for i.MX8ULP FEC, why do we have to have
> > "fsl,imx6q-fec"
> > >>>>> here?
> > >>>>>
> > >>>>> 	fec: ethernet@29950000 {
> > >>>>> 		compatible = "fsl,imx8ulp-fec", "fsl,imx6ul-fec";
> > >>>>> 		...
> > >>>>> 	};
> > >>>>
> > >>>> Because a bit earlier this bindings is saying that fsl,imx6ul-fec
> > >>>> must be followed by fsl,imx6q-fec.
> > >>>
> > >>> The FEC driver OF match table suggests that fsl,imx6ul-fec and
> > >>> fsl,imx6q-fec are not really compatible.
> > >>>
> > >>> static const struct of_device_id fec_dt_ids[] = {
> > >>>         { .compatible = "fsl,imx25-fec", .data =
> > &fec_devtype[IMX25_FEC], },
> > >>>         { .compatible = "fsl,imx27-fec", .data =
> > &fec_devtype[IMX27_FEC], },
> > >>>         { .compatible = "fsl,imx28-fec", .data =
> > &fec_devtype[IMX28_FEC], },
> > >>>         { .compatible = "fsl,imx6q-fec", .data =
> > &fec_devtype[IMX6Q_FEC], },
> > >>>         { .compatible = "fsl,mvf600-fec", .data =
> > &fec_devtype[MVF600_FEC], },
> > >>>         { .compatible = "fsl,imx6sx-fec", .data =
> > &fec_devtype[IMX6SX_FEC], },
> > >>>         { .compatible = "fsl,imx6ul-fec", .data =
> > >>> &fec_devtype[IMX6UL_FEC], },
> > >>
> > >> I don't see here any incompatibility. Binding driver with different
> > >> driver data is not a proof of incompatible devices.
> > >
> > > To me, different driver data is a good sign of incompatibility.  It
> > > mostly means that software needs to program the hardware block
> > > differently.
> >
> > Any device being 100% compatible with old one and having additional
> > features will have different driver data... so no, it's not a proof.
> > There are many of such examples and we call them compatible, because
> > the device could operate when bound by the fallback compatible.
> >
> > If this is the case here - how do I know? I raised and the answer was
> > affirmative...
> >
> > >
> > >
> > >> Additionally, the
> > >> binding describes the hardware, not the driver.
> > >>
> > >>>         { .compatible = "fsl,imx8mq-fec", .data =
> > &fec_devtype[IMX8MQ_FEC], },
> > >>>         { .compatible = "fsl,imx8qm-fec", .data =
> > &fec_devtype[IMX8QM_FEC], },
> > >>>         { /* sentinel */ }
> > >>> };
> > >>> MODULE_DEVICE_TABLE(of, fec_dt_ids);
> > >>>
> > >>> Should we fix the binding doc?
> > >>
> > >> Maybe, I don't know. The binding describes the hardware, so based
> > >> on it the devices are compatible. Changing this, except ABI impact,
> > >> would be possible with proper reason, but not based on Linux driver
> code.
> > >
> > > Well, if Linux driver code is written in the way that hardware
> > > requires, I guess that's just based on hardware characteristics.
> > >
> > > To me, having a device compatible to two devices that require
> > > different programming model is unnecessary and confusing.
> >
> > It's the first time anyone mentions here the programming model is
> > different... If it is different, the devices are likely not compatible.
> >
> > However when I raised this issue last time, there were no concerns
> > with calling them all compatible. Therefore I wonder if the folks
> > working on this driver actually know what's there... I don't know, I
> > gave recommendations based on what is described in the binding and
> > expect the engineer to come with that knowledge.
> >
> >
> Sorry, I did not explain clearly last time, I just mentioned that imx8ulp fec
> was totally reused from imx6ul and was a little different from imx6q.
> So what should I do next? Should I fix the binding doc ?

Just my understanding, saying i.MX6Q supports feature A,
i.MX6UL support feature A + B, Then i.MX6UL is compatible with i.MX6Q.

If upper is true from hardware level, then i.MX8ULP FEC node
should contain 8ulp, 6ul, 6q.

But the list may expand too long if more and more devices are supported
and hardware backward compatible

Regards,
Peng.

Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Krzysztof Kozlowski]

On 19/08/2022 06:13, Peng Fan wrote:
>>>
>> Sorry, I did not explain clearly last time, I just mentioned that imx8ulp fec
>> was totally reused from imx6ul and was a little different from imx6q.
>> So what should I do next? Should I fix the binding doc ?
> 
> Just my understanding, saying i.MX6Q supports feature A,
> i.MX6UL support feature A + B, Then i.MX6UL is compatible with i.MX6Q.

Or if i.MX8ULP can bind with any previous compatible and still work
(with limited subset of features).

> 
> If upper is true from hardware level, then i.MX8ULP FEC node
> should contain 8ulp, 6ul, 6q.
> 
> But the list may expand too long if more and more devices are supported
> and hardware backward compatible

True. I guess three items is the limit and anything newer should restart
the sequence.

Best regards,
Krzysztof

RE: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Wei Fang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: 2022年8月19日 14:32
> To: Peng Fan <peng.fan@nxp.com>; Wei Fang <wei.fang@nxp.com>; Shawn
> Guo <shawnguo@kernel.org>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> s.hauer@pengutronix.de; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; festevam@gmail.com;
> dl-linux-imx <linux-imx@nxp.com>; Jacky Bai <ping.bai@nxp.com>;
> sudeep.holla@arm.com; linux-arm-kernel@lists.infradead.org; Aisheng Dong
> <aisheng.dong@nxp.com>
> Subject: Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item
> 
> On 19/08/2022 06:13, Peng Fan wrote:
> >>>
> >> Sorry, I did not explain clearly last time, I just mentioned that
> >> imx8ulp fec was totally reused from imx6ul and was a little different from
> imx6q.
> >> So what should I do next? Should I fix the binding doc ?
> >
> > Just my understanding, saying i.MX6Q supports feature A, i.MX6UL
> > support feature A + B, Then i.MX6UL is compatible with i.MX6Q.
> 
> Or if i.MX8ULP can bind with any previous compatible and still work (with
> limited subset of features).
> 
> >
> > If upper is true from hardware level, then i.MX8ULP FEC node should
> > contain 8ulp, 6ul, 6q.
> >
> > But the list may expand too long if more and more devices are
> > supported and hardware backward compatible
> 
> True. I guess three items is the limit and anything newer should restart the
> sequence.
> 

So, the binding doc doesn't need to be fixed ?