[PATCH] Requesting review of U-Boot mxl-8611x device tree bindings

[PATCH] Requesting review of U-Boot mxl-8611x device tree bindings

[Nate Drude]

I am working on adding a driver for the mxl-8611x ethernet PHY to 
U-Boot. While there is a Linux driver from the vendor, it is not ready 
for mainline and it does not currently have any device tree bindings.

The U-Boot maintainers requested that I have the device tree bindings 
reviewed by the Linux devicetree@ to avoid any misalignment between 
U-Boot and Linux when the driver lands in Linux.

I will send the bindings U-Boot patch In-Reply-To this email.

Thanks,
Nate

[PATCH] dt-bindings: add device tree bindings for mxl-8611x PHY

[Nate Drude]

The MXL8611X driver has custom bindings for configuring the LEDs
and RGMII internal delays. This patch adds the documentation and
defines necessary to configure it from the device tree.

Signed-off-by: Nate Drude <nate.d@variscite.com>
---
  .../net/phy/mxl-8611x.txt                     | 37 +++++++++++++++++++
  include/dt-bindings/net/mxl-8611x.h           | 27 ++++++++++++++
  2 files changed, 64 insertions(+)
  create mode 100644 doc/device-tree-bindings/net/phy/mxl-8611x.txt
  create mode 100644 include/dt-bindings/net/mxl-8611x.h

diff --git a/doc/device-tree-bindings/net/phy/mxl-8611x.txt 
b/doc/device-tree-bindings/net/phy/mxl-8611x.txt
new file mode 100644
index 00000000000..462fdf61666
--- /dev/null
+++ b/doc/device-tree-bindings/net/phy/mxl-8611x.txt
@@ -0,0 +1,37 @@
+* MaxLinear MXL8611x PHY Device Tree binding
+
+Required properties:
+- reg: PHY address
+
+Optional properties:
+- mxl-8611x,ledN_cfg: Register configuration for COM_EXT_LED0_CFG,
+	COM_EXT_LED1_CFG, and COM_EXT_LED2_CFG
+- mxl-8611x,rx-internal-delay-ps: RGMII RX Clock Delay used only when 
PHY operates
+	in RGMII mode with internal delay (phy-mode is 'rgmii-id' or
+	'rgmii-rxid') in pico-seconds.
+- mxl-8611x,tx-internal-delay-ps-100m: RGMII TX Clock Delay used only 
when PHY operates
+	in 10/100M RGMII mode with internal delay (phy-mode is 'rgmii-id' or
+	'rgmii-txid') in pico-seconds.
+- mxl-8611x,tx-internal-delay-ps-1g: RGMII TX Clock Delay used only 
when PHY operates
+	in 1G RGMII mode with internal delay (phy-mode is 'rgmii-id' or
+	'rgmii-txid') in pico-seconds.
+
+Example:
+
+	ethernet-phy@0 {
+		reg = <0>;
+
+		mxl-8611x,led0_cfg = <(
+			MXL8611X_LEDX_CFG_LINK_UP_RX_ACT_ON |
+			MXL8611X_LEDX_CFG_LINK_UP_TX_ACT_ON |
+			MXL8611X_LEDX_CFG_TRAFFIC_ACT_BLINK_IND
+		)>;
+		mxl-8611x,led1_cfg = <(
+			MXL8611X_LEDX_CFG_LINK_UP_10MB_ON |
+			MXL8611X_LEDX_CFG_LINK_UP_100MB_ON |
+			MXL8611X_LEDX_CFG_LINK_UP_1GB_ON
+		)>;
+		mxl-8611x,rx-internal-delay-ps = <0>;
+		mxl-8611x,tx-internal-delay-ps-100m = <2250>;
+		mxl-8611x,tx-internal-delay-ps-1g = <150>;
+	};
diff --git a/include/dt-bindings/net/mxl-8611x.h 
b/include/dt-bindings/net/mxl-8611x.h
new file mode 100644
index 00000000000..cb0ec0f8bd0
--- /dev/null
+++ b/include/dt-bindings/net/mxl-8611x.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Device Tree constants for MaxLinear MXL8611x PHYs
+ *
+ * Copyright 2023 Variscite Ltd.
+ * Copyright 2023 MaxLinear Inc.
+ */
+
+#ifndef _DT_BINDINGS_MXL_8611X_H
+#define _DT_BINDINGS_MXL_8611X_H
+
+#define MXL8611X_LEDX_CFG_TRAFFIC_ACT_BLINK_IND		(1 << 13)
+#define MXL8611X_LEDX_CFG_LINK_UP_FULL_DUPLEX_ON	(1 << 12)
+#define MXL8611X_LEDX_CFG_LINK_UP_HALF_DUPLEX_ON	(1 << 11)
+#define MXL8611X_LEDX_CFG_LINK_UP_TX_ACT_ON		(1 << 10)
+#define MXL8611X_LEDX_CFG_LINK_UP_RX_ACT_ON		(1 << 9)
+#define MXL8611X_LEDX_CFG_LINK_UP_TX_ON			(1 << 8)
+#define MXL8611X_LEDX_CFG_LINK_UP_RX_ON			(1 << 7)
+#define MXL8611X_LEDX_CFG_LINK_UP_1GB_ON		(1 << 6)
+#define MXL8611X_LEDX_CFG_LINK_UP_100MB_ON		(1 << 5)
+#define MXL8611X_LEDX_CFG_LINK_UP_10MB_ON		(1 << 4)
+#define MXL8611X_LEDX_CFG_LINK_UP_COLLISION		(1 << 3)
+#define MXL8611X_LEDX_CFG_LINK_UP_1GB_BLINK		(1 << 2)
+#define MXL8611X_LEDX_CFG_LINK_UP_100MB_BLINK		(1 << 1)
+#define MXL8611X_LEDX_CFG_LINK_UP_10MB_BLINK		(1 << 0)
+
+#endif
-- 
2.40.1

Re: [PATCH] dt-bindings: add device tree bindings for mxl-8611x PHY

[Krzysztof Kozlowski]

On 13/07/2023 15:23, Nate Drude wrote:
> The MXL8611X driver has custom bindings for configuring the LEDs
> and RGMII internal delays. This patch adds the documentation and
> defines necessary to configure it from the device tree.
> 
> Signed-off-by: Nate Drude <nate.d@variscite.com>
> ---
>   .../net/phy/mxl-8611x.txt                     | 37 +++++++++++++++++++
>   include/dt-bindings/net/mxl-8611x.h           | 27 ++++++++++++++
>   2 files changed, 64 insertions(+)
>   create mode 100644 doc/device-tree-bindings/net/phy/mxl-8611x.txt

You did not CC DT maintainers, so kind of funny way to ask them to
review something.

In general I will not provide full review for projects other than Linux.
Submit bindings to the Linux, following proper Linux kernel process, if
you wish them to be fully reviewed.


>   create mode 100644 include/dt-bindings/net/mxl-8611x.h
> 
> diff --git a/doc/device-tree-bindings/net/phy/mxl-8611x.txt 
> b/doc/device-tree-bindings/net/phy/mxl-8611x.txt
> new file mode 100644
> index 00000000000..462fdf61666
> --- /dev/null
> +++ b/doc/device-tree-bindings/net/phy/mxl-8611x.txt
> @@ -0,0 +1,37 @@
> +* MaxLinear MXL8611x PHY Device Tree binding
> +
> +Required properties:
> +- reg: PHY address
> +
> +Optional properties:
> +- mxl-8611x,ledN_cfg: Register configuration for COM_EXT_LED0_CFG,

That's not correct vendor name. Neither property name - underscores are
not allowed. The property itself does not describe any feature or
hardware. We do not program registers in DT.

> +	COM_EXT_LED1_CFG, and COM_EXT_LED2_CFG
> +- mxl-8611x,rx-internal-delay-ps: RGMII RX Clock Delay used only when 
> PHY operates
> +	in RGMII mode with internal delay (phy-mode is 'rgmii-id' or
> +	'rgmii-rxid') in pico-seconds.
> +- mxl-8611x,tx-internal-delay-ps-100m: RGMII TX Clock Delay used only 

Use correct unit suffixes.

> when PHY operates
> +	in 10/100M RGMII mode with internal delay (phy-mode is 'rgmii-id' or
> +	'rgmii-txid') in pico-seconds.
> +- mxl-8611x,tx-internal-delay-ps-1g: RGMII TX Clock Delay used only 
> when PHY operates
> +	in 1G RGMII mode with internal delay (phy-mode is 'rgmii-id' or
> +	'rgmii-txid') in pico-seconds.

Same problem.

> +
> +Example:
> +
> +	ethernet-phy@0 {
> +		reg = <0>;
> +
> +		mxl-8611x,led0_cfg = <(
> +			MXL8611X_LEDX_CFG_LINK_UP_RX_ACT_ON |
> +			MXL8611X_LEDX_CFG_LINK_UP_TX_ACT_ON |
> +			MXL8611X_LEDX_CFG_TRAFFIC_ACT_BLINK_IND
> +		)>;
> +		mxl-8611x,led1_cfg = <(
> +			MXL8611X_LEDX_CFG_LINK_UP_10MB_ON |
> +			MXL8611X_LEDX_CFG_LINK_UP_100MB_ON |
> +			MXL8611X_LEDX_CFG_LINK_UP_1GB_ON
> +		)>;
> +		mxl-8611x,rx-internal-delay-ps = <0>;
> +		mxl-8611x,tx-internal-delay-ps-100m = <2250>;
> +		mxl-8611x,tx-internal-delay-ps-1g = <150>;
> +	};
> diff --git a/include/dt-bindings/net/mxl-8611x.h 
> b/include/dt-bindings/net/mxl-8611x.h
> new file mode 100644
> index 00000000000..cb0ec0f8bd0
> --- /dev/null
> +++ b/include/dt-bindings/net/mxl-8611x.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +/*
> + * Device Tree constants for MaxLinear MXL8611x PHYs
> + *
> + * Copyright 2023 Variscite Ltd.
> + * Copyright 2023 MaxLinear Inc.
> + */
> +
> +#ifndef _DT_BINDINGS_MXL_8611X_H
> +#define _DT_BINDINGS_MXL_8611X_H
> +
> +#define MXL8611X_LEDX_CFG_TRAFFIC_ACT_BLINK_IND		(1 << 13)

Register values are not bindings.


Best regards,
Krzysztof

Re: [PATCH] dt-bindings: add device tree bindings for mxl-8611x PHY

[Nate Drude]

Hi Krzysztof,

On 7/13/23 2:06 PM, Krzysztof Kozlowski wrote:
> On 13/07/2023 15:23, Nate Drude wrote:
>> The MXL8611X driver has custom bindings for configuring the LEDs
>> and RGMII internal delays. This patch adds the documentation and
>> defines necessary to configure it from the device tree.
>>
>> Signed-off-by: Nate Drude <nate.d@variscite.com>
>> ---
>>    .../net/phy/mxl-8611x.txt                     | 37 +++++++++++++++++++
>>    include/dt-bindings/net/mxl-8611x.h           | 27 ++++++++++++++
>>    2 files changed, 64 insertions(+)
>>    create mode 100644 doc/device-tree-bindings/net/phy/mxl-8611x.txt
> 
> You did not CC DT maintainers, so kind of funny way to ask them to
> review something.
> 
> In general I will not provide full review for projects other than Linux.
> Submit bindings to the Linux, following proper Linux kernel process, if
> you wish them to be fully reviewed.

I appreciate your review, and sorry for the CC mistake. I added the 
other maintainers.

> 
> 
>>    create mode 100644 include/dt-bindings/net/mxl-8611x.h
>>
>> diff --git a/doc/device-tree-bindings/net/phy/mxl-8611x.txt
>> b/doc/device-tree-bindings/net/phy/mxl-8611x.txt
>> new file mode 100644
>> index 00000000000..462fdf61666
>> --- /dev/null
>> +++ b/doc/device-tree-bindings/net/phy/mxl-8611x.txt
>> @@ -0,0 +1,37 @@
>> +* MaxLinear MXL8611x PHY Device Tree binding
>> +
>> +Required properties:
>> +- reg: PHY address
>> +
>> +Optional properties:
>> +- mxl-8611x,ledN_cfg: Register configuration for COM_EXT_LED0_CFG,
> 
> That's not correct vendor name. Neither property name - underscores are
> not allowed. The property itself does not describe any feature or
> hardware. We do not program registers in DT.

Thanks, I'll need to reconsider how to abstract this from register 
values. Forget about this for now, I will split the LED support into a 
separate effort.

> 
>> +	COM_EXT_LED1_CFG, and COM_EXT_LED2_CFG
>> +- mxl-8611x,rx-internal-delay-ps: RGMII RX Clock Delay used only when
>> PHY operates
>> +	in RGMII mode with internal delay (phy-mode is 'rgmii-id' or
>> +	'rgmii-rxid') in pico-seconds.
>> +- mxl-8611x,tx-internal-delay-ps-100m: RGMII TX Clock Delay used only
> 
> Use correct unit suffixes.
> 
>> when PHY operates
>> +	in 10/100M RGMII mode with internal delay (phy-mode is 'rgmii-id' or
>> +	'rgmii-txid') in pico-seconds.
>> +- mxl-8611x,tx-internal-delay-ps-1g: RGMII TX Clock Delay used only
>> when PHY operates
>> +	in 1G RGMII mode with internal delay (phy-mode is 'rgmii-id' or
>> +	'rgmii-txid') in pico-seconds.
> 
> Same problem.

Can you please provide feedback on these updated bindings?

- maxlinear,rx-internal-delay-ps: RGMII RX Clock Delay used only when 
PHY operates
	in RGMII mode with internal delay (phy-mode is 'rgmii-id' or
	'rgmii-rxid') in pico-seconds.
- maxlinear,tx-internal-delay-100m-ps: RGMII TX Clock Delay used only 
when PHY operates
	in 10/100M RGMII mode with internal delay (phy-mode is 'rgmii-id' or
	'rgmii-txid') in pico-seconds.
- maxlinear,tx-internal-delay-1g-ps: RGMII TX Clock Delay used only when 
PHY operates
	in 1G RGMII mode with internal delay (phy-mode is 'rgmii-id' or
	'rgmii-txid') in pico-seconds.

> 
>> +
>> +Example:
>> +
>> +	ethernet-phy@0 {
>> +		reg = <0>;
>> +
>> +		mxl-8611x,led0_cfg = <(
>> +			MXL8611X_LEDX_CFG_LINK_UP_RX_ACT_ON |
>> +			MXL8611X_LEDX_CFG_LINK_UP_TX_ACT_ON |
>> +			MXL8611X_LEDX_CFG_TRAFFIC_ACT_BLINK_IND
>> +		)>;
>> +		mxl-8611x,led1_cfg = <(
>> +			MXL8611X_LEDX_CFG_LINK_UP_10MB_ON |
>> +			MXL8611X_LEDX_CFG_LINK_UP_100MB_ON |
>> +			MXL8611X_LEDX_CFG_LINK_UP_1GB_ON
>> +		)>;
>> +		mxl-8611x,rx-internal-delay-ps = <0>;
>> +		mxl-8611x,tx-internal-delay-ps-100m = <2250>;
>> +		mxl-8611x,tx-internal-delay-ps-1g = <150>;
>> +	};
>> diff --git a/include/dt-bindings/net/mxl-8611x.h
>> b/include/dt-bindings/net/mxl-8611x.h
>> new file mode 100644
>> index 00000000000..cb0ec0f8bd0
>> --- /dev/null
>> +++ b/include/dt-bindings/net/mxl-8611x.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> +/*
>> + * Device Tree constants for MaxLinear MXL8611x PHYs
>> + *
>> + * Copyright 2023 Variscite Ltd.
>> + * Copyright 2023 MaxLinear Inc.
>> + */
>> +
>> +#ifndef _DT_BINDINGS_MXL_8611X_H
>> +#define _DT_BINDINGS_MXL_8611X_H
>> +
>> +#define MXL8611X_LEDX_CFG_TRAFFIC_ACT_BLINK_IND		(1 << 13)
> 
> Register values are not bindings.
> 
> 
> Best regards,
> Krzysztof
> 

Once again, thanks for your review. Hopefully when the driver patches 
are sent for Linux this will have been helpful.

Sincerely,
Nate

Re: [PATCH] dt-bindings: add device tree bindings for mxl-8611x PHY

[Krzysztof Kozlowski]

On 14/07/2023 14:53, Nate Drude wrote:
> 
> Once again, thanks for your review. Hopefully when the driver patches 
> are sent for Linux this will have been helpful.


Driver patches are not really necessary here and I would say are
independent. So as I said - submit bindings to the Linux kernel, so they
will get proper review. Otherwise it does not matter what I say here. It
does not guarantee absolutely any stable bindings because whatever
someone submits to Linux kernel will later overrule it. Therefore I do
not see any benefit of your approach - you will not be able to create
Linux kernel ABI (bindings) by sending binding patch to U-Boot. It does
not work like that.

Best regards,
Krzysztof