Re: [PATCH v2 1/5] net/ncsi: make one oem_gma function for all mfr id

Re: [PATCH v2 1/5] net/ncsi: make one oem_gma function for all mfr id

[Simon Horman]

On Tue, May 09, 2023 at 02:35:00PM +0000, Ivan Mikhaylov wrote:
> Make the one Get Mac Address function for all manufacturers and change
> this call in handlers accordingly.
> 
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH v2 4/5] net/ncsi: add shift MAC address property

[Simon Horman]

On Tue, May 09, 2023 at 02:35:03PM +0000, Ivan Mikhaylov wrote:
> Add the shift MAC address property for GMA command which provides which
> shift should be used but keep old one values for backward compatability.

nit: s/compatability/compatibility/

> 
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> ---
>  net/ncsi/ncsi-rsp.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 069c2659074b..1f108db34d85 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -9,6 +9,8 @@
>  #include <linux/netdevice.h>
>  #include <linux/etherdevice.h>
>  #include <linux/skbuff.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
>  
>  #include <net/ncsi.h>
>  #include <net/net_namespace.h>
> @@ -616,9 +618,12 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
>  {
>  	struct ncsi_dev_priv *ndp = nr->ndp;
>  	struct net_device *ndev = ndp->ndev.dev;
> +	struct platform_device *pdev;
>  	struct ncsi_rsp_oem_pkt *rsp;
>  	struct sockaddr saddr;
>  	u32 mac_addr_off = 0;
> +	s32 shift_mac_addr = 0;
> +	u64 mac_addr;
>  	int ret = 0;

nit: please preserve reverse xmas tree - longest line to shortest - order
     for local variables in networking code. Or in this case,
     move towards rather than away from that pattern.

>  	/* Get the response header */
> @@ -635,7 +640,17 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
>  
>  	memcpy(saddr.sa_data, &rsp->data[mac_addr_off], ETH_ALEN);
>  	if (mfr_id == NCSI_OEM_MFR_BCM_ID || mfr_id == NCSI_OEM_MFR_INTEL_ID)
> -		eth_addr_inc((u8 *)saddr.sa_data);
> +		shift_mac_addr = 1;
> +
> +	pdev = to_platform_device(ndev->dev.parent);
> +	if (pdev)
> +		of_property_read_s32(pdev->dev.of_node,
> +				     "mac-address-increment", &shift_mac_addr);
> +
> +	/* Increase mac address by shift value for BMC's address */
> +	mac_addr = ether_addr_to_u64((u8 *)saddr.sa_data);
> +	mac_addr += shift_mac_addr;
> +	u64_to_ether_addr(mac_addr, (u8 *)saddr.sa_data);
>  	if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
>  		return -ENXIO;
>  
> -- 
> 2.40.1
> 
> 

[PATCH v2 0/5] Refactoring for GMA command

[Ivan Mikhaylov]

Make one GMA function for all manufacturers, change ndo_set_mac_address
to dev_set_mac_address for notifiying net layer about MAC change which
ndo_set_mac_address doesn't do. 

Add mac-address-increment option for possibility to control MAC address
assignment on BMC via GMA command. 

Changes from v1:
	1. delete ftgmac100.txt changes about mac-address-increment
	2. add convert to yaml from ftgmac100.txt
	3. add mac-address-increment option for ethernet-controller.yaml

Ivan Mikhaylov (5):
  net/ncsi: make one oem_gma function for all mfr id
  net/ncsi: change from ndo_set_mac_address to dev_set_mac_address
  dt-bindings: net: add mac-address-increment option
  net/ncsi: add shift MAC address property
  dt-bindings: net: ftgmac100: convert to yaml version from txt

 .../bindings/net/ethernet-controller.yaml     |   8 ++
 .../bindings/net/faraday,ftgmac100.yaml       | 110 ++++++++++++++++++
 net/ncsi/ncsi-rsp.c                           | 108 ++++++-----------
 3 files changed, 155 insertions(+), 71 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml

-- 
2.40.1

[PATCH v2 1/5] net/ncsi: make one oem_gma function for all mfr id

[Ivan Mikhaylov]

Make the one Get Mac Address function for all manufacturers and change
this call in handlers accordingly.

Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 net/ncsi/ncsi-rsp.c | 88 ++++++++++-----------------------------------
 1 file changed, 19 insertions(+), 69 deletions(-)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 6447a09932f5..91c42253a711 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -611,14 +611,15 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
 	return 0;
 }
 
-/* Response handler for Mellanox command Get Mac Address */
-static int ncsi_rsp_handler_oem_mlx_gma(struct ncsi_request *nr)
+/* Response handler for Get Mac Address command */
+static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 {
 	struct ncsi_dev_priv *ndp = nr->ndp;
 	struct net_device *ndev = ndp->ndev.dev;
 	const struct net_device_ops *ops = ndev->netdev_ops;
 	struct ncsi_rsp_oem_pkt *rsp;
 	struct sockaddr saddr;
+	u32 mac_addr_off = 0;
 	int ret = 0;
 
 	/* Get the response header */
@@ -626,7 +627,19 @@ static int ncsi_rsp_handler_oem_mlx_gma(struct ncsi_request *nr)
 
 	saddr.sa_family = ndev->type;
 	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
-	memcpy(saddr.sa_data, &rsp->data[MLX_MAC_ADDR_OFFSET], ETH_ALEN);
+	if (mfr_id == NCSI_OEM_MFR_BCM_ID)
+		mac_addr_off = BCM_MAC_ADDR_OFFSET;
+	else if (mfr_id == NCSI_OEM_MFR_MLX_ID)
+		mac_addr_off = MLX_MAC_ADDR_OFFSET;
+	else if (mfr_id == NCSI_OEM_MFR_INTEL_ID)
+		mac_addr_off = INTEL_MAC_ADDR_OFFSET;
+
+	memcpy(saddr.sa_data, &rsp->data[mac_addr_off], ETH_ALEN);
+	if (mfr_id == NCSI_OEM_MFR_BCM_ID || mfr_id == NCSI_OEM_MFR_INTEL_ID)
+		eth_addr_inc((u8 *)saddr.sa_data);
+	if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
+		return -ENXIO;
+
 	/* Set the flag for GMA command which should only be called once */
 	ndp->gma_flag = 1;
 
@@ -649,41 +662,10 @@ static int ncsi_rsp_handler_oem_mlx(struct ncsi_request *nr)
 
 	if (mlx->cmd == NCSI_OEM_MLX_CMD_GMA &&
 	    mlx->param == NCSI_OEM_MLX_CMD_GMA_PARAM)
-		return ncsi_rsp_handler_oem_mlx_gma(nr);
+		return ncsi_rsp_handler_oem_gma(nr, NCSI_OEM_MFR_MLX_ID);
 	return 0;
 }
 
-/* Response handler for Broadcom command Get Mac Address */
-static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
-{
-	struct ncsi_dev_priv *ndp = nr->ndp;
-	struct net_device *ndev = ndp->ndev.dev;
-	const struct net_device_ops *ops = ndev->netdev_ops;
-	struct ncsi_rsp_oem_pkt *rsp;
-	struct sockaddr saddr;
-	int ret = 0;
-
-	/* Get the response header */
-	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
-
-	saddr.sa_family = ndev->type;
-	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
-	memcpy(saddr.sa_data, &rsp->data[BCM_MAC_ADDR_OFFSET], ETH_ALEN);
-	/* Increase mac address by 1 for BMC's address */
-	eth_addr_inc((u8 *)saddr.sa_data);
-	if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
-		return -ENXIO;
-
-	/* Set the flag for GMA command which should only be called once */
-	ndp->gma_flag = 1;
-
-	ret = ops->ndo_set_mac_address(ndev, &saddr);
-	if (ret < 0)
-		netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
-
-	return ret;
-}
-
 /* Response handler for Broadcom card */
 static int ncsi_rsp_handler_oem_bcm(struct ncsi_request *nr)
 {
@@ -695,42 +677,10 @@ static int ncsi_rsp_handler_oem_bcm(struct ncsi_request *nr)
 	bcm = (struct ncsi_rsp_oem_bcm_pkt *)(rsp->data);
 
 	if (bcm->type == NCSI_OEM_BCM_CMD_GMA)
-		return ncsi_rsp_handler_oem_bcm_gma(nr);
+		return ncsi_rsp_handler_oem_gma(nr, NCSI_OEM_MFR_BCM_ID);
 	return 0;
 }
 
-/* Response handler for Intel command Get Mac Address */
-static int ncsi_rsp_handler_oem_intel_gma(struct ncsi_request *nr)
-{
-	struct ncsi_dev_priv *ndp = nr->ndp;
-	struct net_device *ndev = ndp->ndev.dev;
-	const struct net_device_ops *ops = ndev->netdev_ops;
-	struct ncsi_rsp_oem_pkt *rsp;
-	struct sockaddr saddr;
-	int ret = 0;
-
-	/* Get the response header */
-	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
-
-	saddr.sa_family = ndev->type;
-	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
-	memcpy(saddr.sa_data, &rsp->data[INTEL_MAC_ADDR_OFFSET], ETH_ALEN);
-	/* Increase mac address by 1 for BMC's address */
-	eth_addr_inc((u8 *)saddr.sa_data);
-	if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
-		return -ENXIO;
-
-	/* Set the flag for GMA command which should only be called once */
-	ndp->gma_flag = 1;
-
-	ret = ops->ndo_set_mac_address(ndev, &saddr);
-	if (ret < 0)
-		netdev_warn(ndev,
-			    "NCSI: 'Writing mac address to device failed\n");
-
-	return ret;
-}
-
 /* Response handler for Intel card */
 static int ncsi_rsp_handler_oem_intel(struct ncsi_request *nr)
 {
@@ -742,7 +692,7 @@ static int ncsi_rsp_handler_oem_intel(struct ncsi_request *nr)
 	intel = (struct ncsi_rsp_oem_intel_pkt *)(rsp->data);
 
 	if (intel->cmd == NCSI_OEM_INTEL_CMD_GMA)
-		return ncsi_rsp_handler_oem_intel_gma(nr);
+		return ncsi_rsp_handler_oem_gma(nr, NCSI_OEM_MFR_INTEL_ID);
 
 	return 0;
 }
-- 
2.40.1

[PATCH v2 2/5] net/ncsi: change from ndo_set_mac_address to dev_set_mac_address

[Ivan Mikhaylov]

Change ndo_set_mac_address to dev_set_mac_address because
dev_set_mac_address provides a way to notify network layer about MAC
change. In other case, services may not aware about MAC change and keep
using old one which set from network adapter driver.

As example, DHCP client from systemd do not update MAC address without
notification from net subsystem which leads to the problem with acquiring
the right address from DHCP server.

Signed-off-by: Paul Fertser <fercerpav@gmail.com>
Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 net/ncsi/ncsi-rsp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 91c42253a711..069c2659074b 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -616,7 +616,6 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 {
 	struct ncsi_dev_priv *ndp = nr->ndp;
 	struct net_device *ndev = ndp->ndev.dev;
-	const struct net_device_ops *ops = ndev->netdev_ops;
 	struct ncsi_rsp_oem_pkt *rsp;
 	struct sockaddr saddr;
 	u32 mac_addr_off = 0;
@@ -643,7 +642,9 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 	/* Set the flag for GMA command which should only be called once */
 	ndp->gma_flag = 1;
 
-	ret = ops->ndo_set_mac_address(ndev, &saddr);
+	rtnl_lock();
+	ret = dev_set_mac_address(ndev, &saddr, NULL);
+	rtnl_unlock();
 	if (ret < 0)
 		netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
 
-- 
2.40.1

[PATCH v2 3/5] dt-bindings: net: add mac-address-increment option

[Ivan Mikhaylov]

Add the mac-address-increment option for specify MAC address taken by
any other sources.

Signed-off-by: Paul Fertser <fercerpav@gmail.com>
Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 .../devicetree/bindings/net/ethernet-controller.yaml      | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 00be387984ac..6900098c5105 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -34,6 +34,14 @@ properties:
     minItems: 6
     maxItems: 6
 
+  mac-address-increment:
+    $ref: /schemas/types.yaml#/definitions/int32
+    description:
+      Specifies the MAC address increment to be added to the MAC address.
+      Should be used in cases when there is a need to use MAC address
+      different from one obtained by any other level, like u-boot or the
+      NC-SI stack.
+
   max-frame-size:
     $ref: /schemas/types.yaml#/definitions/uint32
     description:
-- 
2.40.1

[PATCH v2 4/5] net/ncsi: add shift MAC address property

[Ivan Mikhaylov]

Add the shift MAC address property for GMA command which provides which
shift should be used but keep old one values for backward compatability.

Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 net/ncsi/ncsi-rsp.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 069c2659074b..1f108db34d85 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -9,6 +9,8 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
 
 #include <net/ncsi.h>
 #include <net/net_namespace.h>
@@ -616,9 +618,12 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 {
 	struct ncsi_dev_priv *ndp = nr->ndp;
 	struct net_device *ndev = ndp->ndev.dev;
+	struct platform_device *pdev;
 	struct ncsi_rsp_oem_pkt *rsp;
 	struct sockaddr saddr;
 	u32 mac_addr_off = 0;
+	s32 shift_mac_addr = 0;
+	u64 mac_addr;
 	int ret = 0;
 
 	/* Get the response header */
@@ -635,7 +640,17 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 
 	memcpy(saddr.sa_data, &rsp->data[mac_addr_off], ETH_ALEN);
 	if (mfr_id == NCSI_OEM_MFR_BCM_ID || mfr_id == NCSI_OEM_MFR_INTEL_ID)
-		eth_addr_inc((u8 *)saddr.sa_data);
+		shift_mac_addr = 1;
+
+	pdev = to_platform_device(ndev->dev.parent);
+	if (pdev)
+		of_property_read_s32(pdev->dev.of_node,
+				     "mac-address-increment", &shift_mac_addr);
+
+	/* Increase mac address by shift value for BMC's address */
+	mac_addr = ether_addr_to_u64((u8 *)saddr.sa_data);
+	mac_addr += shift_mac_addr;
+	u64_to_ether_addr(mac_addr, (u8 *)saddr.sa_data);
 	if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
 		return -ENXIO;
 
-- 
2.40.1

[PATCH v2 5/5] dt-bindings: net: ftgmac100: convert to yaml version from txt

[Ivan Mikhaylov]

Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 .../bindings/net/faraday,ftgmac100.yaml       | 110 ++++++++++++++++++
 1 file changed, 110 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml

diff --git a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
new file mode 100644
index 000000000000..98cd142f74bb
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
@@ -0,0 +1,110 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/faraday,ftgmac100.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Faraday Technology FTGMAC100 gigabit ethernet controller
+
+allOf:
+  - $ref: "ethernet-controller.yaml#"
+
+maintainers:
+  - Po-Yu Chuang <ratbert@faraday-tech.com>
+
+properties:
+  compatible:
+    oneOf:
+      - const: faraday,ftgmac100
+      - items:
+          - enum:
+              - aspeed,ast2400-mac
+              - aspeed,ast2500-mac
+              - aspeed,ast2600-mac
+          - const: faraday,ftgmac100
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    description: |
+      In accordance with the generic clock bindings. Must describe the MAC
+      IP clock, and optionally an RMII RCLK gate for the AST2500/AST2600. The
+      required MAC clock must be the first cell.
+    minItems: 1
+    maxItems: 2
+
+  clock-names:
+    items:
+      - enum:
+          - MACCLK
+          - RCLK
+
+  phy-mode:
+    enum:
+      - rgmii
+      - rmii
+
+  phy-handle: true
+
+  use-ncsi:
+    description: |
+      Use the NC-SI stack instead of an MDIO PHY. Currently assumes
+      rmii (100bT) but kept as a separate property in case NC-SI grows support
+      for a gigabit link.
+    type: boolean
+
+  no-hw-checksum:
+    description: |
+      Used to disable HW checksum support. Here for backward
+      compatibility as the driver now should have correct defaults based on
+      the SoC.
+    type: boolean
+
+  mac-address-increment:
+    description: |
+      Increment the MAC address taken by GMA command via  NC-SI. Specifies
+      a signed number to be added to the host MAC address as obtained by
+      the OEM GMA command. If not specified, 1 is used by default for
+      Broadcom and Intel network cards, 0 otherwise.
+
+  mdio:
+    $ref: /schemas/net/mdio.yaml#
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    mac0: ethernet@1e660000 {
+        compatible = "aspeed,ast2500-mac", "faraday,ftgmac100";
+        reg = <0x1e660000 0x180>;
+        interrupts = <2>;
+        use-ncsi;
+    };
+
+    mac1: ethernet@1e680000 {
+        compatible = "aspeed,ast2500-mac", "faraday,ftgmac100";
+        reg = <0x1e680000 0x180>;
+        interrupts = <2>;
+
+        phy-handle = <&phy>;
+        phy-mode = "rgmii";
+
+        mdio {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            phy: ethernet-phy@1 {
+                compatible = "ethernet-phy-ieee802.3-c22";
+                reg = <1>;
+            };
+        };
+    };
-- 
2.40.1

Re: [PATCH v2 3/5] dt-bindings: net: add mac-address-increment option

[Krzysztof Kozlowski]

On 09/05/2023 16:35, Ivan Mikhaylov wrote:
> Add the mac-address-increment option for specify MAC address taken by
> any other sources.
> 
> Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> ---
>  .../devicetree/bindings/net/ethernet-controller.yaml      | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> index 00be387984ac..6900098c5105 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -34,6 +34,14 @@ properties:
>      minItems: 6
>      maxItems: 6
>  
> +  mac-address-increment:
> +    $ref: /schemas/types.yaml#/definitions/int32
> +    description:
> +      Specifies the MAC address increment to be added to the MAC address.
> +      Should be used in cases when there is a need to use MAC address
> +      different from one obtained by any other level, like u-boot or the
> +      NC-SI stack.

We don't store MAC addresses in DT, but provide simple placeholder for
firmware or bootloader. Why shall we store static "increment" part of
MAC address? Can't the firmware give you proper MAC address?

Best regards,
Krzysztof

Re: [PATCH v2 5/5] dt-bindings: net: ftgmac100: convert to yaml version from txt

[Krzysztof Kozlowski]

On 09/05/2023 16:35, Ivan Mikhaylov wrote:
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>

Need some commit msg.


> ---
>  .../bindings/net/faraday,ftgmac100.yaml       | 110 ++++++++++++++++++

Missing actual conversion (removal).

>  1 file changed, 110 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> new file mode 100644
> index 000000000000..98cd142f74bb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual-license, unless you copied some chunks of old binding... but was
there old binding?

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/faraday,ftgmac100.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Faraday Technology FTGMAC100 gigabit ethernet controller
> +
> +allOf:
> +  - $ref: "ethernet-controller.yaml#"

Drop quotes.


> +
> +maintainers:
> +  - Po-Yu Chuang <ratbert@faraday-tech.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: faraday,ftgmac100
> +      - items:
> +          - enum:
> +              - aspeed,ast2400-mac
> +              - aspeed,ast2500-mac
> +              - aspeed,ast2600-mac
> +          - const: faraday,ftgmac100
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    description: |
> +      In accordance with the generic clock bindings.

Drop this part. Obvious.

>  Must describe the MAC
> +      IP clock, and optionally an RMII RCLK gate for the AST2500/AST2600. The
> +      required MAC clock must be the first cell.

The cells depend on clock provider. Do you mean something else?

> +    minItems: 1
> +    maxItems: 2
> +
> +  clock-names:
> +    items:
> +      - enum:
> +          - MACCLK
> +          - RCLK

This does not allow two clocks... List all the items and add minItems: 1.


> +
> +  phy-mode:
> +    enum:
> +      - rgmii
> +      - rmii
> +
> +  phy-handle: true
> +
> +  use-ncsi:
> +    description: |

Do not need '|' unless you need to preserve formatting.

I will stop review, because it depends whether this is true conversion
or new binding.

Best regards,
Krzysztof

Re: [PATCH v2 3/5] dt-bindings: net: add mac-address-increment option

[Ivan Mikhaylov]

On Wed, 2023-05-10 at 16:48 +0200, Krzysztof Kozlowski wrote:
> On 09/05/2023 16:35, Ivan Mikhaylov wrote:
> > Add the mac-address-increment option for specify MAC address taken
> > by
> > any other sources.
> > 
> > Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> > ---
> >  .../devicetree/bindings/net/ethernet-controller.yaml      | 8
> > ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ethernet-
> > controller.yaml b/Documentation/devicetree/bindings/net/ethernet-
> > controller.yaml
> > index 00be387984ac..6900098c5105 100644
> > --- a/Documentation/devicetree/bindings/net/ethernet-
> > controller.yaml
> > +++ b/Documentation/devicetree/bindings/net/ethernet-
> > controller.yaml
> > @@ -34,6 +34,14 @@ properties:
> >      minItems: 6
> >      maxItems: 6
> >  
> > +  mac-address-increment:
> > +    $ref: /schemas/types.yaml#/definitions/int32
> > +    description:
> > +      Specifies the MAC address increment to be added to the MAC
> > address.
> > +      Should be used in cases when there is a need to use MAC
> > address
> > +      different from one obtained by any other level, like u-boot
> > or the
> > +      NC-SI stack.
> 
> We don't store MAC addresses in DT, but provide simple placeholder
> for
> firmware or bootloader. Why shall we store static "increment" part of
> MAC address? Can't the firmware give you proper MAC address?
> 
> Best regards,
> Krzysztof
> 

Krzysztof, maybe that's a point to make commit message with better
explanation from my side. At current time there is at least two cases
where I see it's possible to be used:

1. NC-SI
2. embedded

At NC-SI level there is Get Mac Address command which provides to BMC
mac address from the host which is same as host mac address, it happens
at runtime and overrides old one.

Also, this part was also to be discussed 2 years ago in this thread:
https://lore.kernel.org/all/OF8E108F72.39D22E89-ON00258765.001E46EB-00258765.00251157@ibm.com/

Where Milton provided this information:

DTMF spec DSP0222 NC-SI (network controller sideband interface)
is a method to provide a BMC (Baseboard management controller) shared
access to an external ethernet port for comunication to the management
network in the outside world.  The protocol describes ethernet packets 
that control selective bridging implemented in a host network
controller
to share its phy.  Various NIC OEMs have added a query to find out the 
address the host is using, and some vendors have added code to query
host
nic and set the BMC mac to a fixed offset (current hard coded +1 from
the host value).  If this is compiled in the kernel, the NIC OEM is 
recognised and the BMC doesn't miss the NIC response the address is set
once each time the NCSI stack reinitializes.  This mechanism overrides
any mac-address or local-mac-address or other assignment.

DSP0222
https://www.dmtf.org/documents/pmci/network-controller-sideband-interface-nc-si-specification-110


In embedded case, sometimes you have different multiple ethernet
interfaces which using one mac address which increments or decrements
for particular interface, just for better explanation, there is patch
with explanation which providing them such way of work:
https://github.com/openwrt/openwrt/blob/master/target/linux/generic/pending-5.15/682-of_net-add-mac-address-increment-support.patch

In their rep a lot of dts using such option.

Thanks.

Re: [PATCH v2 5/5] dt-bindings: net: ftgmac100: convert to yaml version from txt

[Ivan Mikhaylov]

On Wed, 2023-05-10 at 16:50 +0200, Krzysztof Kozlowski wrote:
> On 09/05/2023 16:35, Ivan Mikhaylov wrote:
> > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> 
> Need some commit msg.
> 
> 
> > ---
> >  .../bindings/net/faraday,ftgmac100.yaml       | 110
> > ++++++++++++++++++
> 
> Missing actual conversion (removal).
> 
> >  1 file changed, 110 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> > b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> > new file mode 100644
> > index 000000000000..98cd142f74bb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> > @@ -0,0 +1,110 @@
> > +# SPDX-License-Identifier: GPL-2.0
> 
> Dual-license, unless you copied some chunks of old binding... but was
> there old binding?

Krzysztof, I copied a lot from old one ftgmac100.txt.

> 
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/faraday,ftgmac100.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Faraday Technology FTGMAC100 gigabit ethernet controller
> > +
> > +allOf:
> > +  - $ref: "ethernet-controller.yaml#"
> 
> Drop quotes.
> 
> 
> > +
> > +maintainers:
> > +  - Po-Yu Chuang <ratbert@faraday-tech.com>
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - const: faraday,ftgmac100
> > +      - items:
> > +          - enum:
> > +              - aspeed,ast2400-mac
> > +              - aspeed,ast2500-mac
> > +              - aspeed,ast2600-mac
> > +          - const: faraday,ftgmac100
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    description: |
> > +      In accordance with the generic clock bindings.
> 
> Drop this part. Obvious.
> 
> >  Must describe the MAC
> > +      IP clock, and optionally an RMII RCLK gate for the
> > AST2500/AST2600. The
> > +      required MAC clock must be the first cell.
> 
> The cells depend on clock provider. Do you mean something else?

It's exactly from ftgmac100.txt, this section without any changes, I
didn't add anything new in the sentence.

> 
> > +    minItems: 1
> > +    maxItems: 2
> > +
> > +  clock-names:
> > +    items:
> > +      - enum:
> > +          - MACCLK
> > +          - RCLK
> 
> This does not allow two clocks... List all the items and add
> minItems: 1.
> 
> 
> > +
> > +  phy-mode:
> > +    enum:
> > +      - rgmii
> > +      - rmii
> > +
> > +  phy-handle: true
> > +
> > +  use-ncsi:
> > +    description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
> I will stop review, because it depends whether this is true
> conversion
> or new binding.
> 

I've tried to convert it from ftgmac100.txt, everything is same in it
except mac-address-increment option with explanation for which purpose
it there, need I divide conversion from that option or is it fine in
one?

Thanks.

Re: [PATCH v2 5/5] dt-bindings: net: ftgmac100: convert to yaml version from txt

[Krzysztof Kozlowski]

On 11/05/2023 02:15, Ivan Mikhaylov wrote:
>>> +  phy-mode:
>>> +    enum:
>>> +      - rgmii
>>> +      - rmii
>>> +
>>> +  phy-handle: true
>>> +
>>> +  use-ncsi:
>>> +    description: |
>>
>> Do not need '|' unless you need to preserve formatting.
>>
>> I will stop review, because it depends whether this is true
>> conversion
>> or new binding.
>>
> 
> I've tried to convert it from ftgmac100.txt, everything is same in it
> except mac-address-increment option with explanation for which purpose
> it there, need I divide conversion from that option or is it fine in
> one?

As I wrote at beginning, I don't see conversion here, so difficult to judge.

Best regards,
Krzysztof

Re: [PATCH v2 3/5] dt-bindings: net: add mac-address-increment option

[Krzysztof Kozlowski]

On 11/05/2023 01:31, Ivan Mikhaylov wrote:
> On Wed, 2023-05-10 at 16:48 +0200, Krzysztof Kozlowski wrote:
>> On 09/05/2023 16:35, Ivan Mikhaylov wrote:
>>> Add the mac-address-increment option for specify MAC address taken
>>> by
>>> any other sources.
>>>
>>> Signed-off-by: Paul Fertser <fercerpav@gmail.com>
>>> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
>>> ---
>>>  .../devicetree/bindings/net/ethernet-controller.yaml      | 8
>>> ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/ethernet-
>>> controller.yaml b/Documentation/devicetree/bindings/net/ethernet-
>>> controller.yaml
>>> index 00be387984ac..6900098c5105 100644
>>> --- a/Documentation/devicetree/bindings/net/ethernet-
>>> controller.yaml
>>> +++ b/Documentation/devicetree/bindings/net/ethernet-
>>> controller.yaml
>>> @@ -34,6 +34,14 @@ properties:
>>>      minItems: 6
>>>      maxItems: 6
>>>  
>>> +  mac-address-increment:
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>> +    description:
>>> +      Specifies the MAC address increment to be added to the MAC
>>> address.
>>> +      Should be used in cases when there is a need to use MAC
>>> address
>>> +      different from one obtained by any other level, like u-boot
>>> or the
>>> +      NC-SI stack.
>>
>> We don't store MAC addresses in DT, but provide simple placeholder
>> for
>> firmware or bootloader. Why shall we store static "increment" part of
>> MAC address? Can't the firmware give you proper MAC address?
>>
>> Best regards,
>> Krzysztof
>>
> 
> Krzysztof, maybe that's a point to make commit message with better
> explanation from my side. At current time there is at least two cases
> where I see it's possible to be used:
> 
> 1. NC-SI
> 2. embedded
> 
> At NC-SI level there is Get Mac Address command which provides to BMC
> mac address from the host which is same as host mac address, it happens
> at runtime and overrides old one.
> 
> Also, this part was also to be discussed 2 years ago in this thread:
> https://lore.kernel.org/all/OF8E108F72.39D22E89-ON00258765.001E46EB-00258765.00251157@ibm.com/

Which was not sent to Rob though...


> 
> Where Milton provided this information:
> 
> DTMF spec DSP0222 NC-SI (network controller sideband interface)
> is a method to provide a BMC (Baseboard management controller) shared
> access to an external ethernet port for comunication to the management
> network in the outside world.  The protocol describes ethernet packets 
> that control selective bridging implemented in a host network
> controller
> to share its phy.  Various NIC OEMs have added a query to find out the 
> address the host is using, and some vendors have added code to query
> host
> nic and set the BMC mac to a fixed offset (current hard coded +1 from
> the host value).  If this is compiled in the kernel, the NIC OEM is 
> recognised and the BMC doesn't miss the NIC response the address is set
> once each time the NCSI stack reinitializes.  This mechanism overrides
> any mac-address or local-mac-address or other assignment.
> 
> DSP0222
> https://www.dmtf.org/documents/pmci/network-controller-sideband-interface-nc-si-specification-110
> 
> 
> In embedded case, sometimes you have different multiple ethernet
> interfaces which using one mac address which increments or decrements
> for particular interface, just for better explanation, there is patch
> with explanation which providing them such way of work:
> https://github.com/openwrt/openwrt/blob/master/target/linux/generic/pending-5.15/682-of_net-add-mac-address-increment-support.patch
> 
> In their rep a lot of dts using such option.

None of these explain why this is property of the hardware. I understand
that this is something you want Linux to do, but DT is not for that
purpose. Do not encode system policies into DT and what above commit
says is a policy.

Best regards,
Krzysztof

Re: [PATCH v2 3/5] dt-bindings: net: add mac-address-increment option

[Krzysztof Kozlowski]

On 12/05/2023 13:28, Ivan Mikhaylov wrote:
> On Fri, 2023-05-12 at 08:22 +0200, Krzysztof Kozlowski wrote:
>> On 11/05/2023 01:31, Ivan Mikhaylov wrote:
>>> On Wed, 2023-05-10 at 16:48 +0200, Krzysztof Kozlowski wrote:
>>>> On 09/05/2023 16:35, Ivan Mikhaylov wrote:
>>>>> Add the mac-address-increment option for specify MAC address
>>>>> taken
>>>>> by
>>>>> any other sources.
>>>>>
>>>>> Signed-off-by: Paul Fertser <fercerpav@gmail.com>
>>>>> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
>>>>> ---
>>>>>  .../devicetree/bindings/net/ethernet-controller.yaml      | 8
>>>>> ++++++++
>>>>>  1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/net/ethernet-
>>>>> controller.yaml
>>>>> b/Documentation/devicetree/bindings/net/ethernet-
>>>>> controller.yaml
>>>>> index 00be387984ac..6900098c5105 100644
>>>>> --- a/Documentation/devicetree/bindings/net/ethernet-
>>>>> controller.yaml
>>>>> +++ b/Documentation/devicetree/bindings/net/ethernet-
>>>>> controller.yaml
>>>>> @@ -34,6 +34,14 @@ properties:
>>>>>      minItems: 6
>>>>>      maxItems: 6
>>>>>  
>>>>> +  mac-address-increment:
>>>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>>>> +    description:
>>>>> +      Specifies the MAC address increment to be added to the
>>>>> MAC
>>>>> address.
>>>>> +      Should be used in cases when there is a need to use MAC
>>>>> address
>>>>> +      different from one obtained by any other level, like u-
>>>>> boot
>>>>> or the
>>>>> +      NC-SI stack.
>>>>
>>>> We don't store MAC addresses in DT, but provide simple
>>>> placeholder
>>>> for
>>>> firmware or bootloader. Why shall we store static "increment"
>>>> part of
>>>> MAC address? Can't the firmware give you proper MAC address?
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> Krzysztof, maybe that's a point to make commit message with better
>>> explanation from my side. At current time there is at least two
>>> cases
>>> where I see it's possible to be used:
>>>
>>> 1. NC-SI
>>> 2. embedded
>>>
>>> At NC-SI level there is Get Mac Address command which provides to
>>> BMC
>>> mac address from the host which is same as host mac address, it
>>> happens
>>> at runtime and overrides old one.
>>>
>>> Also, this part was also to be discussed 2 years ago in this
>>> thread:
>>> https://lore.kernel.org/all/OF8E108F72.39D22E89-ON00258765.001E46EB-00258765.00251157@ibm.com/
>>
>> Which was not sent to Rob though...
>>
>>
>>>
>>> Where Milton provided this information:
>>>
>>> DTMF spec DSP0222 NC-SI (network controller sideband interface)
>>> is a method to provide a BMC (Baseboard management controller)
>>> shared
>>> access to an external ethernet port for comunication to the
>>> management
>>> network in the outside world.  The protocol describes ethernet
>>> packets 
>>> that control selective bridging implemented in a host network
>>> controller
>>> to share its phy.  Various NIC OEMs have added a query to find out
>>> the 
>>> address the host is using, and some vendors have added code to
>>> query
>>> host
>>> nic and set the BMC mac to a fixed offset (current hard coded +1
>>> from
>>> the host value).  If this is compiled in the kernel, the NIC OEM is
>>> recognised and the BMC doesn't miss the NIC response the address is
>>> set
>>> once each time the NCSI stack reinitializes.  This mechanism
>>> overrides
>>> any mac-address or local-mac-address or other assignment.
>>>
>>> DSP0222
>>> https://www.dmtf.org/documents/pmci/network-controller-sideband-interface-nc-si-specification-110
>>>
>>>
>>> In embedded case, sometimes you have different multiple ethernet
>>> interfaces which using one mac address which increments or
>>> decrements
>>> for particular interface, just for better explanation, there is
>>> patch
>>> with explanation which providing them such way of work:
>>> https://github.com/openwrt/openwrt/blob/master/target/linux/generic/pending-5.15/682-of_net-add-mac-address-increment-support.patch
>>>
>>> In their rep a lot of dts using such option.
>>
>> None of these explain why this is property of the hardware. I
>> understand
>> that this is something you want Linux to do, but DT is not for that
>> purpose. Do not encode system policies into DT and what above commit
>> says is a policy.
>>
> 
> Krzysztof, okay then to which DT subsystem it should belong? To
> ftgmac100 after conversion?

To my understanding, decision to add some numbers to MAC address does
not look like DT property at all. Otherwise please help me to understand
- why different boards with same device should have different offset/value?

Anyway, commit msg also lacks any justification for this.

Best regards,
Krzysztof

Re: [PATCH v2 3/5] dt-bindings: net: add mac-address-increment option

[Ivan Mikhaylov]

On Fri, 2023-05-12 at 08:22 +0200, Krzysztof Kozlowski wrote:
> On 11/05/2023 01:31, Ivan Mikhaylov wrote:
> > On Wed, 2023-05-10 at 16:48 +0200, Krzysztof Kozlowski wrote:
> > > On 09/05/2023 16:35, Ivan Mikhaylov wrote:
> > > > Add the mac-address-increment option for specify MAC address
> > > > taken
> > > > by
> > > > any other sources.
> > > > 
> > > > Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> > > > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> > > > ---
> > > >  .../devicetree/bindings/net/ethernet-controller.yaml      | 8
> > > > ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/net/ethernet-
> > > > controller.yaml
> > > > b/Documentation/devicetree/bindings/net/ethernet-
> > > > controller.yaml
> > > > index 00be387984ac..6900098c5105 100644
> > > > --- a/Documentation/devicetree/bindings/net/ethernet-
> > > > controller.yaml
> > > > +++ b/Documentation/devicetree/bindings/net/ethernet-
> > > > controller.yaml
> > > > @@ -34,6 +34,14 @@ properties:
> > > >      minItems: 6
> > > >      maxItems: 6
> > > >  
> > > > +  mac-address-increment:
> > > > +    $ref: /schemas/types.yaml#/definitions/int32
> > > > +    description:
> > > > +      Specifies the MAC address increment to be added to the
> > > > MAC
> > > > address.
> > > > +      Should be used in cases when there is a need to use MAC
> > > > address
> > > > +      different from one obtained by any other level, like u-
> > > > boot
> > > > or the
> > > > +      NC-SI stack.
> > > 
> > > We don't store MAC addresses in DT, but provide simple
> > > placeholder
> > > for
> > > firmware or bootloader. Why shall we store static "increment"
> > > part of
> > > MAC address? Can't the firmware give you proper MAC address?
> > > 
> > > Best regards,
> > > Krzysztof
> > > 
> > 
> > Krzysztof, maybe that's a point to make commit message with better
> > explanation from my side. At current time there is at least two
> > cases
> > where I see it's possible to be used:
> > 
> > 1. NC-SI
> > 2. embedded
> > 
> > At NC-SI level there is Get Mac Address command which provides to
> > BMC
> > mac address from the host which is same as host mac address, it
> > happens
> > at runtime and overrides old one.
> > 
> > Also, this part was also to be discussed 2 years ago in this
> > thread:
> > https://lore.kernel.org/all/OF8E108F72.39D22E89-ON00258765.001E46EB-00258765.00251157@ibm.com/
> 
> Which was not sent to Rob though...
> 
> 
> > 
> > Where Milton provided this information:
> > 
> > DTMF spec DSP0222 NC-SI (network controller sideband interface)
> > is a method to provide a BMC (Baseboard management controller)
> > shared
> > access to an external ethernet port for comunication to the
> > management
> > network in the outside world.  The protocol describes ethernet
> > packets 
> > that control selective bridging implemented in a host network
> > controller
> > to share its phy.  Various NIC OEMs have added a query to find out
> > the 
> > address the host is using, and some vendors have added code to
> > query
> > host
> > nic and set the BMC mac to a fixed offset (current hard coded +1
> > from
> > the host value).  If this is compiled in the kernel, the NIC OEM is
> > recognised and the BMC doesn't miss the NIC response the address is
> > set
> > once each time the NCSI stack reinitializes.  This mechanism
> > overrides
> > any mac-address or local-mac-address or other assignment.
> > 
> > DSP0222
> > https://www.dmtf.org/documents/pmci/network-controller-sideband-interface-nc-si-specification-110
> > 
> > 
> > In embedded case, sometimes you have different multiple ethernet
> > interfaces which using one mac address which increments or
> > decrements
> > for particular interface, just for better explanation, there is
> > patch
> > with explanation which providing them such way of work:
> > https://github.com/openwrt/openwrt/blob/master/target/linux/generic/pending-5.15/682-of_net-add-mac-address-increment-support.patch
> > 
> > In their rep a lot of dts using such option.
> 
> None of these explain why this is property of the hardware. I
> understand
> that this is something you want Linux to do, but DT is not for that
> purpose. Do not encode system policies into DT and what above commit
> says is a policy.
> 

Krzysztof, okay then to which DT subsystem it should belong? To
ftgmac100 after conversion?

Thanks.

Re: [PATCH v2 3/5] dt-bindings: net: add mac-address-increment option

[Ivan Mikhaylov]

On Fri, 2023-05-12 at 11:24 +0200, Krzysztof Kozlowski wrote:
> On 12/05/2023 13:28, Ivan Mikhaylov wrote:
> > On Fri, 2023-05-12 at 08:22 +0200, Krzysztof Kozlowski wrote:
> > > On 11/05/2023 01:31, Ivan Mikhaylov wrote:
> > > > On Wed, 2023-05-10 at 16:48 +0200, Krzysztof Kozlowski wrote:
> > > > > On 09/05/2023 16:35, Ivan Mikhaylov wrote:
> > > > > > Add the mac-address-increment option for specify MAC
> > > > > > address
> > > > > > taken
> > > > > > by
> > > > > > any other sources.
> > > > > > 
> > > > > > Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> > > > > > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> > > > > > ---
> > > > > >  .../devicetree/bindings/net/ethernet-controller.yaml     
> > > > > > | 8
> > > > > > ++++++++
> > > > > >  1 file changed, 8 insertions(+)
> > > > > > 
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/net/ethernet-
> > > > > > controller.yaml
> > > > > > b/Documentation/devicetree/bindings/net/ethernet-
> > > > > > controller.yaml
> > > > > > index 00be387984ac..6900098c5105 100644
> > > > > > --- a/Documentation/devicetree/bindings/net/ethernet-
> > > > > > controller.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/net/ethernet-
> > > > > > controller.yaml
> > > > > > @@ -34,6 +34,14 @@ properties:
> > > > > >      minItems: 6
> > > > > >      maxItems: 6
> > > > > >  
> > > > > > +  mac-address-increment:
> > > > > > +    $ref: /schemas/types.yaml#/definitions/int32
> > > > > > +    description:
> > > > > > +      Specifies the MAC address increment to be added to
> > > > > > the
> > > > > > MAC
> > > > > > address.
> > > > > > +      Should be used in cases when there is a need to use
> > > > > > MAC
> > > > > > address
> > > > > > +      different from one obtained by any other level, like
> > > > > > u-
> > > > > > boot
> > > > > > or the
> > > > > > +      NC-SI stack.
> > > > > 
> > > > > We don't store MAC addresses in DT, but provide simple
> > > > > placeholder
> > > > > for
> > > > > firmware or bootloader. Why shall we store static "increment"
> > > > > part of
> > > > > MAC address? Can't the firmware give you proper MAC address?
> > > > > 
> > > > > Best regards,
> > > > > Krzysztof
> > > > > 
> > > > 
> > > > Krzysztof, maybe that's a point to make commit message with
> > > > better
> > > > explanation from my side. At current time there is at least two
> > > > cases
> > > > where I see it's possible to be used:
> > > > 
> > > > 1. NC-SI
> > > > 2. embedded
> > > > 
> > > > At NC-SI level there is Get Mac Address command which provides
> > > > to
> > > > BMC
> > > > mac address from the host which is same as host mac address, it
> > > > happens
> > > > at runtime and overrides old one.
> > > > 
> > > > Also, this part was also to be discussed 2 years ago in this
> > > > thread:
> > > > https://lore.kernel.org/all/OF8E108F72.39D22E89-ON00258765.001E46EB-00258765.00251157@ibm.com/
> > > 
> > > Which was not sent to Rob though...
> > > 
> > > 
> > > > 
> > > > Where Milton provided this information:
> > > > 
> > > > DTMF spec DSP0222 NC-SI (network controller sideband interface)
> > > > is a method to provide a BMC (Baseboard management controller)
> > > > shared
> > > > access to an external ethernet port for comunication to the
> > > > management
> > > > network in the outside world.  The protocol describes ethernet
> > > > packets 
> > > > that control selective bridging implemented in a host network
> > > > controller
> > > > to share its phy.  Various NIC OEMs have added a query to find
> > > > out
> > > > the 
> > > > address the host is using, and some vendors have added code to
> > > > query
> > > > host
> > > > nic and set the BMC mac to a fixed offset (current hard coded
> > > > +1
> > > > from
> > > > the host value).  If this is compiled in the kernel, the NIC
> > > > OEM is
> > > > recognised and the BMC doesn't miss the NIC response the
> > > > address is
> > > > set
> > > > once each time the NCSI stack reinitializes.  This mechanism
> > > > overrides
> > > > any mac-address or local-mac-address or other assignment.
> > > > 
> > > > DSP0222
> > > > https://www.dmtf.org/documents/pmci/network-controller-sideband-interface-nc-si-specification-110
> > > > 
> > > > 
> > > > In embedded case, sometimes you have different multiple
> > > > ethernet
> > > > interfaces which using one mac address which increments or
> > > > decrements
> > > > for particular interface, just for better explanation, there is
> > > > patch
> > > > with explanation which providing them such way of work:
> > > > https://github.com/openwrt/openwrt/blob/master/target/linux/generic/pending-5.15/682-of_net-add-mac-address-increment-support.patch
> > > > 
> > > > In their rep a lot of dts using such option.
> > > 
> > > None of these explain why this is property of the hardware. I
> > > understand
> > > that this is something you want Linux to do, but DT is not for
> > > that
> > > purpose. Do not encode system policies into DT and what above
> > > commit
> > > says is a policy.
> > > 
> > 
> > Krzysztof, okay then to which DT subsystem it should belong? To
> > ftgmac100 after conversion?
> 
> To my understanding, decision to add some numbers to MAC address does
> not look like DT property at all. Otherwise please help me to
> understand
> - why different boards with same device should have different
> offset/value?
> 
> Anyway, commit msg also lacks any justification for this.
> 
> Best regards,
> Krzysztof
> 

Krzysztof, essentially some PCIe network cards have like an additional
*MII interface which connects directly to a BMC (separate SoC for
managing a motherboard) and by sending special ethernet type frames
over that connection (called NC-SI) the BMC can obtain MAC, get link
parameters etc. So it's natural for a vendor to allocate two MACs per
such a board with PCIe card intergrated, with one MAC "flashed into"
the network card, under the assumption that the BMC should
automatically use the next MAC. So it's the property of the hardware as
the vendor designs it, not a matter of usage policy.

Also at the nvmem binding tree is "nvmem-cell-cells" which is literally
the same as what was proposed but on different level.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/nvmem?id=7e2805c203a6c8dc85c1cfda205161ed39ae82d5


Thanks.

Re: [PATCH v2 3/5] dt-bindings: net: add mac-address-increment option

[Krzysztof Kozlowski]

On 16/05/2023 13:47, Ivan Mikhaylov wrote:
hy this is property of the hardware. I
>>>> understand
>>>> that this is something you want Linux to do, but DT is not for
>>>> that
>>>> purpose. Do not encode system policies into DT and what above
>>>> commit
>>>> says is a policy.
>>>>
>>>
>>> Krzysztof, okay then to which DT subsystem it should belong? To
>>> ftgmac100 after conversion?
>>
>> To my understanding, decision to add some numbers to MAC address does
>> not look like DT property at all. Otherwise please help me to
>> understand
>> - why different boards with same device should have different
>> offset/value?
>>
>> Anyway, commit msg also lacks any justification for this.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Krzysztof, essentially some PCIe network cards have like an additional
> *MII interface which connects directly to a BMC (separate SoC for
> managing a motherboard) and by sending special ethernet type frames
> over that connection (called NC-SI) the BMC can obtain MAC, get link
> parameters etc. So it's natural for a vendor to allocate two MACs per
> such a board with PCIe card intergrated, with one MAC "flashed into"
> the network card, under the assumption that the BMC should

Who makes the assumption that next MAC should differ by 1 or 2?

> automatically use the next MAC. So it's the property of the hardware as
> the vendor designs it, not a matter of usage policy.
> 
> Also at the nvmem binding tree is "nvmem-cell-cells" which is literally
> the same as what was proposed but on different level.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/nvmem?id=7e2805c203a6c8dc85c1cfda205161ed39ae82d5

How is this similar? This points the location of mac address on some NV
storage. You add fixed value which should be added to the Ethernet.

I might be missing the context but there is no DTS example nor user of
this property, so how can I get such?

Best regards,
Krzysztof

Re: [PATCH v2 3/5] dt-bindings: net: add mac-address-increment option

[Krzysztof Kozlowski]

On 17/05/2023 23:38, Ivan Mikhaylov wrote:
> On Wed, 2023-05-17 at 10:36 +0200, Krzysztof Kozlowski wrote:
>> On 16/05/2023 13:47, Ivan Mikhaylov wrote:
>> hy this is property of the hardware. I
>>>>>> understand
>>>>>> that this is something you want Linux to do, but DT is not
>>>>>> for
>>>>>> that
>>>>>> purpose. Do not encode system policies into DT and what above
>>>>>> commit
>>>>>> says is a policy.
>>>>>>
>>>>>
>>>>> Krzysztof, okay then to which DT subsystem it should belong? To
>>>>> ftgmac100 after conversion?
>>>>
>>>> To my understanding, decision to add some numbers to MAC address
>>>> does
>>>> not look like DT property at all. Otherwise please help me to
>>>> understand
>>>> - why different boards with same device should have different
>>>> offset/value?

I would like to remind this question.
"why different boards with same device should have different offset/value?"

It was literally ignored and you started explaining network cards and
BMC. I don't understand why, but it does not help your case.

Let me extend this question with one more:
"Why for all your boards of one type, so using the same DTS, would you
use one value of incrementing MAC address?"

>>>>
>>>> Anyway, commit msg also lacks any justification for this.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> Krzysztof, essentially some PCIe network cards have like an
>>> additional
>>> *MII interface which connects directly to a BMC (separate SoC for
>>> managing a motherboard) and by sending special ethernet type frames
>>> over that connection (called NC-SI) the BMC can obtain MAC, get
>>> link
>>> parameters etc. So it's natural for a vendor to allocate two MACs
>>> per
>>> such a board with PCIe card intergrated, with one MAC "flashed
>>> into"
>>> the network card, under the assumption that the BMC should
>>
>> Who makes the assumption that next MAC should differ by 1 or 2?
> 
> Krzysztof, in this above case BMC does, BMC should care about changing
> it and doing it with current codebase without any options just by some
> hardcoded numbers which is wrong.

But you hard-code the number, just in BMC DTS. How does it differ from
BMC hard-coding it differently?

You encode policy - or software decisions - into Devicetree.

> 
>>
>>> automatically use the next MAC. So it's the property of the
>>> hardware as
>>> the vendor designs it, not a matter of usage policy.
>>>
>>> Also at the nvmem binding tree is "nvmem-cell-cells" which is
>>> literally
>>> the same as what was proposed but on different level.
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/nvmem?id=7e2805c203a6c8dc85c1cfda205161ed39ae82d5
>>
>> How is this similar? This points the location of mac address on some
>> NV
>> storage. You add fixed value which should be added to the Ethernet.
> 
> It's not the points the location, this particular option provides this
> increment for mac addresses to make use of them with multiple
> interfaces. Just part of above commit:
> "It's used as a base for calculating addresses for multiple interfaces.
> It's done by adding proper values. Actual offsets are picked by
> manufacturers and vary across devices."
> 
> It is same as we talked before about mac-address-increment in openwrt
> project, if you want examples, you can look into their github. And same
> as we trying to achieve here.
> 
> https://github.com/openwrt/openwrt/blob/master/target/linux/generic/pending-5.15/682-of_net-add-mac-address-increment-support.patch

Awesome... so if project added wrong property to bindings, e.g. SW
property, you find it as an argument for anyone else.

No, that's not how it works.

> 
> "Lots of embedded devices use the mac-address of other interface
> extracted from nvmem cells and increments it by one or two. Add two
> bindings to integrate this and directly use the right mac-address for
> the interface. Some example are some routers that use the gmac
> mac-address stored in the art partition and increments it by one for
> the
> wifi. mac-address-increment-byte bindings is used to tell what byte of
> the mac-address has to be increased (if not defined the last byte is
> increased) and mac-address-increment tells how much the byte decided
> early has to be increased."
> 
> Don't you see similarity with nvmem commit?

Explanation is similar, but you are using wrong argument to justify the
property. The MAC address is stored in some NVMEM cell. There is such
NVMEM cell. That's the hardware property, thus it is justified in DT.

Now how MAC address will be modified - by 1, 2, 3, 252 - is not related
to that commit, because it is a software decision.

Again, we are back to the previous question to which you answered "BMC
will do it". I understand this is property for the BMC DTS, thus:
Why for all your boards of one type, so using one DTS, would you use one
value of incrementing MAC address?
Why devices with same board cannot use different values? One board "1"
and second "2" for MAC increments? I am sure that one customer could
have it different.

The choice how much you increment some MAC address is not a hardware
property. It does not even look like a firmware property. If playing
with this property was done by firmware, like we do for all MAC address
fields, then I would expect here some references to it. Which you did
not provide, I believe.



> 
>>
>> I might be missing the context but there is no DTS example nor user
>> of
>> this property, so how can I get such?
>>
> 
> I don't see it either in linux kernel DTS tree but it in DTS doc.
> 
> Also, just a little bit history about older propositions
> https://lore.kernel.org/all/?q=mac-address-increment
> https://lore.kernel.org/all/20200919214941.8038-5-ansuelsmth@gmail.com/

I don't see any user there, except the same rejected proposal:

https://lore.kernel.org/all/CAL_JsqKhyeh2=pJcpBKkh+s3FM__DY+VoYSYJLRUErrujTLn9A@mail.gmail.com/

If you want to convince us, please illustrate it in a real world
upstreamed DTS (or explain why it cannot). Otherwise I don't see
justification as it is not a hardware property.

This is a NAK from me.

Feel free to ping Rob in some later time, as he might have different
opinion.

Best regards,
Krzysztof

Re: [PATCH v2 3/5] dt-bindings: net: add mac-address-increment option

[Ivan Mikhaylov]

On Wed, 2023-05-17 at 10:36 +0200, Krzysztof Kozlowski wrote:
> On 16/05/2023 13:47, Ivan Mikhaylov wrote:
> hy this is property of the hardware. I
> > > > > understand
> > > > > that this is something you want Linux to do, but DT is not
> > > > > for
> > > > > that
> > > > > purpose. Do not encode system policies into DT and what above
> > > > > commit
> > > > > says is a policy.
> > > > > 
> > > > 
> > > > Krzysztof, okay then to which DT subsystem it should belong? To
> > > > ftgmac100 after conversion?
> > > 
> > > To my understanding, decision to add some numbers to MAC address
> > > does
> > > not look like DT property at all. Otherwise please help me to
> > > understand
> > > - why different boards with same device should have different
> > > offset/value?
> > > 
> > > Anyway, commit msg also lacks any justification for this.
> > > 
> > > Best regards,
> > > Krzysztof
> > > 
> > 
> > Krzysztof, essentially some PCIe network cards have like an
> > additional
> > *MII interface which connects directly to a BMC (separate SoC for
> > managing a motherboard) and by sending special ethernet type frames
> > over that connection (called NC-SI) the BMC can obtain MAC, get
> > link
> > parameters etc. So it's natural for a vendor to allocate two MACs
> > per
> > such a board with PCIe card intergrated, with one MAC "flashed
> > into"
> > the network card, under the assumption that the BMC should
> 
> Who makes the assumption that next MAC should differ by 1 or 2?

Krzysztof, in this above case BMC does, BMC should care about changing
it and doing it with current codebase without any options just by some
hardcoded numbers which is wrong.

> 
> > automatically use the next MAC. So it's the property of the
> > hardware as
> > the vendor designs it, not a matter of usage policy.
> > 
> > Also at the nvmem binding tree is "nvmem-cell-cells" which is
> > literally
> > the same as what was proposed but on different level.
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/nvmem?id=7e2805c203a6c8dc85c1cfda205161ed39ae82d5
> 
> How is this similar? This points the location of mac address on some
> NV
> storage. You add fixed value which should be added to the Ethernet.

It's not the points the location, this particular option provides this
increment for mac addresses to make use of them with multiple
interfaces. Just part of above commit:
"It's used as a base for calculating addresses for multiple interfaces.
It's done by adding proper values. Actual offsets are picked by
manufacturers and vary across devices."

It is same as we talked before about mac-address-increment in openwrt
project, if you want examples, you can look into their github. And same
as we trying to achieve here.

https://github.com/openwrt/openwrt/blob/master/target/linux/generic/pending-5.15/682-of_net-add-mac-address-increment-support.patch

"Lots of embedded devices use the mac-address of other interface
extracted from nvmem cells and increments it by one or two. Add two
bindings to integrate this and directly use the right mac-address for
the interface. Some example are some routers that use the gmac
mac-address stored in the art partition and increments it by one for
the
wifi. mac-address-increment-byte bindings is used to tell what byte of
the mac-address has to be increased (if not defined the last byte is
increased) and mac-address-increment tells how much the byte decided
early has to be increased."

Don't you see similarity with nvmem commit?

> 
> I might be missing the context but there is no DTS example nor user
> of
> this property, so how can I get such?
> 

I don't see it either in linux kernel DTS tree but it in DTS doc.

Also, just a little bit history about older propositions
https://lore.kernel.org/all/?q=mac-address-increment
https://lore.kernel.org/all/20200919214941.8038-5-ansuelsmth@gmail.com/


Thanks.

Re: [PATCH v2 3/5] dt-bindings: net: add mac-address-increment option

[Paul Fertser]

Hello Krzysztof,

Let me try to clarify a bit on the particular usecase and answer your
questions.

Let's consider a server motherboard manufactured and sold by a single
company. This motherboard includes I210 (Ethernet Controlleer) chip
along with the other necessary parts right there, soldered to the PCB,
non-replaceable. This I210 is connected to the host CPU with a PCIe
lane and acts as a regular network adapter. In addition to that this
chip is connected using NC-SI (management channel) to the BMC SoC
(also permanently soldered to the board).

There is a separate EEPROM connected directly to I210 which hosts its
firmware and many operational parameters, including the MAC
address. This EEPROM is not anyhow accessible by the BMC (the host can
read/write it using special protocol over PCIe). Intel expects the
board manufacturer to embed a MAC address from the manufacturer's
range in the EEPROM configuration. But in many cases it's desirable to
use a separate MAC address for the BMC (then I210 acts as if it has an
integrated switch), so the board manufacturer can, by its internal
policy, allocate two consecutive MAC addresses to each motherboard.

The only way BMC can learn the MAC address used by I210 is by a
special vendor-specific NC-SI command, and it can provide just a
single address, the one used by the host. NC-SI is using Ethernet
frames with a special type, so to execute this command the network
driver needs to be (at least partially) functional. I do not really
imagine nvmem getting support to read it this way.

On Wed, May 17, 2023 at 09:26:35PM +0200, Krzysztof Kozlowski wrote:
> I would like to remind this question.
> "why different boards with same device should have different offset/value?"

In the usecase we're aiming for the DT is describing a specific board
from manufacturer that guarantees the offset to be correct, as none of
the parts are replaceable and the MAC address is flashed into the
I210 EEPROM during manufacturing.

> Let me extend this question with one more:
> "Why for all your boards of one type, so using the same DTS, would you
> use one value of incrementing MAC address?"

Here we assume that for all the boards supported by a particular DT
the board manufacturer guarantees the MAC address offset by internal
production policy, by allocating the addresses from the manufacturer's
pool.

> But you hard-code the number, just in BMC DTS. How does it differ from
> BMC hard-coding it differently?
> 
> You encode policy - or software decisions - into Devicetree.

But MAC address of an Ethernet equipment is an inherent part of the
hardware. It's just that we can't store it in an nvmem-addressable
cell in this case, unfortunately.

> Why devices with same board cannot use different values? One board "1"
> and second "2" for MAC increments? I am sure that one customer could
> have it different.

You assume that the customers might be allocating their own MAC
addresses for the network interface of a motherboard, that might be
true if the customer gets such a board from an ODM. But such a
customer not willing to follow the MAC address offsets policy is not
much different from a customer who e.g. modifies flash partitions or
storage format making the nvmem references invalid, and so requiring a
separate DT.

> If you want to convince us, please illustrate it in a real world
> upstreamed DTS (or explain why it cannot). Otherwise I don't see
> justification as it is not a hardware property.

Can you please tell how you would imagine a responsible vendor tackle
the usecase I outlined? Guess it's not by a startup script that would
be getting a MAC address from an interface, applying the offset, and
then change it on the same interface?

Thank you for the review and discussion.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

Re: [PATCH v2 3/5] dt-bindings: net: add mac-address-increment option

[Krzysztof Kozlowski]

On 29/05/2023 22:59, Paul Fertser wrote:
> Hello Krzysztof,
> 
> Let me try to clarify a bit on the particular usecase and answer your
> questions.
> 
> Let's consider a server motherboard manufactured and sold by a single
> company. This motherboard includes I210 (Ethernet Controlleer) chip
> along with the other necessary parts right there, soldered to the PCB,
> non-replaceable. This I210 is connected to the host CPU with a PCIe
> lane and acts as a regular network adapter. In addition to that this
> chip is connected using NC-SI (management channel) to the BMC SoC
> (also permanently soldered to the board).
> 
> There is a separate EEPROM connected directly to I210 which hosts its
> firmware and many operational parameters, including the MAC
> address. This EEPROM is not anyhow accessible by the BMC (the host can
> read/write it using special protocol over PCIe). Intel expects the
> board manufacturer to embed a MAC address from the manufacturer's
> range in the EEPROM configuration. But in many cases it's desirable to
> use a separate MAC address for the BMC (then I210 acts as if it has an
> integrated switch), so the board manufacturer can, by its internal
> policy, allocate two consecutive MAC addresses to each motherboard.
> 
> The only way BMC can learn the MAC address used by I210 is by a
> special vendor-specific NC-SI command, and it can provide just a
> single address, the one used by the host. NC-SI is using Ethernet
> frames with a special type, so to execute this command the network
> driver needs to be (at least partially) functional. I do not really
> imagine nvmem getting support to read it this way.
> 
> On Wed, May 17, 2023 at 09:26:35PM +0200, Krzysztof Kozlowski wrote:
>> I would like to remind this question.
>> "why different boards with same device should have different offset/value?"
> 
> In the usecase we're aiming for the DT is describing a specific board
> from manufacturer that guarantees the offset to be correct, as none of
> the parts are replaceable and the MAC address is flashed into the
> I210 EEPROM during manufacturing.
> 
>> Let me extend this question with one more:
>> "Why for all your boards of one type, so using the same DTS, would you
>> use one value of incrementing MAC address?"
> 
> Here we assume that for all the boards supported by a particular DT
> the board manufacturer guarantees the MAC address offset by internal
> production policy, by allocating the addresses from the manufacturer's
> pool.

OK, embed such information in the commit or property description.

> 
>> But you hard-code the number, just in BMC DTS. How does it differ from
>> BMC hard-coding it differently?
>>
>> You encode policy - or software decisions - into Devicetree.
> 
> But MAC address of an Ethernet equipment is an inherent part of the
> hardware. It's just that we can't store it in an nvmem-addressable
> cell in this case, unfortunately.
> 
>> Why devices with same board cannot use different values? One board "1"
>> and second "2" for MAC increments? I am sure that one customer could
>> have it different.
> 
> You assume that the customers might be allocating their own MAC
> addresses for the network interface of a motherboard, that might be
> true if the customer gets such a board from an ODM. But such a
> customer not willing to follow the MAC address offsets policy is not
> much different from a customer who e.g. modifies flash partitions or
> storage format making the nvmem references invalid, and so requiring a
> separate DT.
> 
>> If you want to convince us, please illustrate it in a real world
>> upstreamed DTS (or explain why it cannot). Otherwise I don't see
>> justification as it is not a hardware property.
> 
> Can you please tell how you would imagine a responsible vendor tackle
> the usecase I outlined?

I would imagine him to upstream the DTS. I asked yo illustrate it. There
is still no DTS user for it so I have doubts it is used as intended.

> Guess it's not by a startup script that would
> be getting a MAC address from an interface, applying the offset, and
> then change it on the same interface?
> 
> Thank you for the review and discussion.
> 

Best regards,
Krzysztof