[PATCH v1 0/2] net: stmmac: add NPCM dwmac support

[PATCH v1 0/2] net: stmmac: add NPCM dwmac support

[Tomer Maimon]

This patch set add dwmac/stmmac for the NPCM Baseboard
Management Controllers (BMC).

NPCM8xx driver is a part of glue logic dwmac driver to support sgmii.

The NPCM dwmac was tested on NPCM845 evaluation board.

Tomer Maimon (2):
  dt-bindings: net: Add support NPCM dwmac
  net: stmmac: Add NPCM support

 .../bindings/net/nuvoton,npcm8xx-sgmii.yaml   |  72 +++++++++++
 .../devicetree/bindings/net/snps,dwmac.yaml   |   1 +
 MAINTAINERS                                   |   1 +
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |   9 ++
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
 .../net/ethernet/stmicro/stmmac/dwmac-npcm.c  | 121 ++++++++++++++++++
 6 files changed, 205 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/nuvoton,npcm8xx-sgmii.yaml
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-npcm.c

-- 
2.33.0

[PATCH v1 1/2] dt-bindings: net: Add support NPCM dwmac

[Tomer Maimon]

Add documentation to describe Nuvoton BMC NPCM dwmac driver(sgmii).

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 .../bindings/net/nuvoton,npcm8xx-sgmii.yaml   | 72 +++++++++++++++++++
 .../devicetree/bindings/net/snps,dwmac.yaml   |  1 +
 2 files changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/nuvoton,npcm8xx-sgmii.yaml

diff --git a/Documentation/devicetree/bindings/net/nuvoton,npcm8xx-sgmii.yaml b/Documentation/devicetree/bindings/net/nuvoton,npcm8xx-sgmii.yaml
new file mode 100644
index 000000000000..6a5f2cade7c9
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/nuvoton,npcm8xx-sgmii.yaml
@@ -0,0 +1,72 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/nuvoton,npcm8xx-sgmii.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton NPCM DWMAC SGMII Ethernet controller
+
+maintainers:
+  - Tomer Maimon <tmaimon77@gmail.com>
+
+select:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - nuvoton,npcm8xx-sgmii
+  required:
+    - compatible
+
+allOf:
+  - $ref: snps,dwmac.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - nuvoton,npcm8xx-sgmii
+          - const: snps,dwmac-3.50a
+  reg:
+    items:
+      - description:
+          The first range represent DWMAC controller registers.
+      - description:
+          The second range represent PCS configuration registers.
+
+  clocks:
+    items:
+      - description: GMAC main clock
+
+  clock-names:
+    items:
+      - const: stmmaceth
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/nuvoton,npcm845-clk.h>
+
+    ahb {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      gmac0: ethernet@f0802000 {
+          compatible = "nuvoton,npcm8xx-sgmii","snps,dwmac-3.50a";
+          reg = <0x0 0xf0802000 0x0 0x2000>, <0x0 0xf0780000 0x0 0x200>;
+          interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
+          interrupt-names = "macirq";
+          clocks = <&clk NPCM8XX_CLK_AHB>;
+          clock-names = "stmmaceth";
+          phy-mode = "rgmii-id";
+      };
+    };
diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index 5c2769dc689a..ba52dbc85144 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -66,6 +66,7 @@ properties:
         - ingenic,x2000-mac
         - loongson,ls2k-dwmac
         - loongson,ls7a-dwmac
+        - nuvoton,npcm8xx-sgmii
         - qcom,qcs404-ethqos
         - qcom,sa8775p-ethqos
         - qcom,sc8280xp-ethqos
-- 
2.33.0

[PATCH v1 2/2] net: stmmac: Add NPCM support

[Tomer Maimon]

Add Nuvoton NPCM BMC SoCs support to STMMAC dwmac driver.

And modify MAINTAINERS to add a new F: entry for this driver.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 MAINTAINERS                                   |   1 +
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |   9 ++
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
 .../net/ethernet/stmicro/stmmac/dwmac-npcm.c  | 121 ++++++++++++++++++
 4 files changed, 132 insertions(+)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-npcm.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5c9f868e13b6..43059c7d00c7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2507,6 +2507,7 @@ F:	arch/arm64/boot/dts/nuvoton/
 F:	drivers/*/*/*npcm*
 F:	drivers/*/*npcm*
 F:	drivers/rtc/rtc-nct3018y.c
+F:	drivers/net/ethernet/stmicro/stmmac/dwmac-npcm.c
 F:	include/dt-bindings/clock/nuvoton,npcm7xx-clock.h
 F:	include/dt-bindings/clock/nuvoton,npcm845-clk.h
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index a2b9e289aa36..2487a674d0d3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -121,6 +121,15 @@ config DWMAC_MESON
 	  the stmmac device driver. This driver is used for Meson6,
 	  Meson8, Meson8b and GXBB SoCs.
 
+config DWMAC_NPCM
+	tristate "Nuvoton NPCM dwmac support"
+	depends on OF && (ARCH_NPCM || COMPILE_TEST)
+	help
+	  Support for Ethernet controller on Nuvoton NPCM BMC SoCs.
+
+	  This selects the Nuvoton NPCM BMC SoC glue layer support for
+	  the stmmac device driver. This driver is used for NPCM8xx SoCs.
+
 config DWMAC_QCOM_ETHQOS
 	tristate "Qualcomm ETHQOS support"
 	default ARCH_QCOM
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index 80e598bd4255..1c86c8ca39f0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_DWMAC_IPQ806X)	+= dwmac-ipq806x.o
 obj-$(CONFIG_DWMAC_LPC18XX)	+= dwmac-lpc18xx.o
 obj-$(CONFIG_DWMAC_MEDIATEK)	+= dwmac-mediatek.o
 obj-$(CONFIG_DWMAC_MESON)	+= dwmac-meson.o dwmac-meson8b.o
+obj-$(CONFIG_DWMAC_NPCM)	+= dwmac-npcm.o
 obj-$(CONFIG_DWMAC_QCOM_ETHQOS)	+= dwmac-qcom-ethqos.o
 obj-$(CONFIG_DWMAC_ROCKCHIP)	+= dwmac-rk.o
 obj-$(CONFIG_DWMAC_SOCFPGA)	+= dwmac-altr-socfpga.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-npcm.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-npcm.c
new file mode 100644
index 000000000000..dbb857661142
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-npcm.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (c) 2023 Nuvoton Technology corporation.
+
+#include <linux/device.h>
+#include <linux/ethtool.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/stmmac.h>
+
+#include "stmmac_platform.h"
+
+#define IND_AC_BA_REG		0x1FE
+#define SR_MII_CTRL		0x3E0000
+
+#define PCS_SR_MII_CTRL_REG	0x0
+#define PCS_SPEED_SELECT6	BIT(6)
+#define PCS_AN_ENABLE		BIT(12)
+#define PCS_SPEED_SELECT13	BIT(13)
+#define PCS_RST			BIT(15)
+
+#define PCS_MASK_SPEED		0xDFBF
+
+struct npcm_dwmac {
+	void __iomem	*reg;
+};
+
+static void npcm_dwmac_fix_mac_speed(void *priv, unsigned int speed,
+				     unsigned int mode)
+{
+	struct npcm_dwmac *dwmac = priv;
+	u16 val;
+
+	iowrite16((u16)(SR_MII_CTRL >> 9), dwmac->reg + IND_AC_BA_REG);
+	val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
+	val &= PCS_MASK_SPEED;
+
+	switch (speed) {
+	case SPEED_1000:
+		val |= PCS_SPEED_SELECT6;
+		break;
+	case SPEED_100:
+		val |= PCS_SPEED_SELECT13;
+		break;
+	case SPEED_10:
+		break;
+	}
+
+	iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
+}
+
+void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
+			 struct plat_stmmacenet_data *plat_dat)
+{
+	u16 val;
+
+	iowrite16((u16)(SR_MII_CTRL >> 9), dwmac->reg + IND_AC_BA_REG);
+	val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
+	val |= PCS_RST;
+	iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
+
+	while (val & PCS_RST)
+		val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
+
+	val &= ~(PCS_AN_ENABLE);
+	iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
+}
+
+static int npcm_dwmac_probe(struct platform_device *pdev)
+{
+	struct plat_stmmacenet_data *plat_dat;
+	struct stmmac_resources stmmac_res;
+	struct npcm_dwmac *dwmac;
+	int ret;
+
+	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
+	if (ret)
+		return ret;
+
+	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	if (IS_ERR(plat_dat))
+		return PTR_ERR(plat_dat);
+
+	dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
+	if (!dwmac)
+		ret = -ENOMEM;
+
+	dwmac->reg = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(dwmac->reg))
+		ret = PTR_ERR(dwmac->reg);
+
+	npcm_dwmac_pcs_init(dwmac, &pdev->dev, plat_dat);
+
+	plat_dat->has_gmac = true;
+	plat_dat->bsp_priv = dwmac;
+	plat_dat->fix_mac_speed = npcm_dwmac_fix_mac_speed;
+
+	return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
+}
+
+static const struct of_device_id npcm_dwmac_match[] = {
+	{ .compatible = "nuvoton,npcm8xx-sgmii" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, npcm_dwmac_match);
+
+static struct platform_driver npcm_dwmac_driver = {
+	.probe  = npcm_dwmac_probe,
+	.remove_new = stmmac_pltfr_remove,
+	.driver = {
+		.name           = "npcm-dwmac",
+		.pm		= &stmmac_pltfr_pm_ops,
+		.of_match_table = npcm_dwmac_match,
+	},
+};
+module_platform_driver(npcm_dwmac_driver);
+
+MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>");
+MODULE_DESCRIPTION("Nuvoton NPCM DWMAC glue layer");
+MODULE_LICENSE("GPL v2");
-- 
2.33.0

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[Russell King (Oracle)]

On Tue, Nov 21, 2023 at 05:17:33PM +0200, Tomer Maimon wrote:
> Add Nuvoton NPCM BMC SoCs support to STMMAC dwmac driver.
> 
> And modify MAINTAINERS to add a new F: entry for this driver.
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>

A few comments on this...

> +#define IND_AC_BA_REG		0x1FE
> +#define SR_MII_CTRL		0x3E0000
> +
> +#define PCS_SR_MII_CTRL_REG	0x0
> +#define PCS_SPEED_SELECT6	BIT(6)
> +#define PCS_AN_ENABLE		BIT(12)
> +#define PCS_SPEED_SELECT13	BIT(13)
> +#define PCS_RST			BIT(15)

include/uapi/linux/mii.h:

#define BMCR_SPEED1000          0x0040  /* MSB of Speed (1000)         */
#define BMCR_ANENABLE           0x1000  /* Enable auto negotiation     */
#define BMCR_SPEED100           0x2000  /* Select 100Mbps              */
#define BMCR_RESET              0x8000  /* Reset to default state      */

Look familiar? Maybe use the standard definitions for a standardised
register?

> +void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
> +			 struct plat_stmmacenet_data *plat_dat)
> +{
> +	u16 val;
> +
> +	iowrite16((u16)(SR_MII_CTRL >> 9), dwmac->reg + IND_AC_BA_REG);
> +	val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> +	val |= PCS_RST;
> +	iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> +
> +	while (val & PCS_RST)
> +		val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);

What if the PCS never clears its reset bit? Maybe use
read_poll_timeout() ?

> +
> +	val &= ~(PCS_AN_ENABLE);
> +	iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> +}

Also, maybe it's time to require new stmmac platform support to start
making use of the phylink PCS support rather than continuing to code its
own?

I notice, however, that you always disable inband signalling - please
explain why. Also, what protocol does the PCS use when communicating
with the PHY?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[Andrew Lunn]

> +void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
> +			 struct plat_stmmacenet_data *plat_dat)
> +{
> +	u16 val;
> +
> +	iowrite16((u16)(SR_MII_CTRL >> 9), dwmac->reg + IND_AC_BA_REG);
> +	val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> +	val |= PCS_RST;
> +	iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> +
> +	while (val & PCS_RST)
> +		val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> +
> +	val &= ~(PCS_AN_ENABLE);
> +	iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> +}

Is this a licensed PCS implementation? Or home grown? If its been
licensed from somebody, it maybe should live in driver/net/pcs, so
others can reuse it when they license the same core.

       Andrew

Re: [PATCH v1 1/2] dt-bindings: net: Add support NPCM dwmac

[Krzysztof Kozlowski]

On 21/11/2023 16:17, Tomer Maimon wrote:
> Add documentation to describe Nuvoton BMC NPCM dwmac driver(sgmii).
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
>  .../bindings/net/nuvoton,npcm8xx-sgmii.yaml   | 72 +++++++++++++++++++
>  .../devicetree/bindings/net/snps,dwmac.yaml   |  1 +
>  2 files changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/nuvoton,npcm8xx-sgmii.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/nuvoton,npcm8xx-sgmii.yaml b/Documentation/devicetree/bindings/net/nuvoton,npcm8xx-sgmii.yaml
> new file mode 100644
> index 000000000000..6a5f2cade7c9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/nuvoton,npcm8xx-sgmii.yaml
> @@ -0,0 +1,72 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/nuvoton,npcm8xx-sgmii.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton NPCM DWMAC SGMII Ethernet controller
> +
> +maintainers:
> +  - Tomer Maimon <tmaimon77@gmail.com>
> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - nuvoton,npcm8xx-sgmii

This must be specific compatible, no wildcards. Then use the same for
filename.

> +  required:
> +    - compatible
> +
> +allOf:
> +  - $ref: snps,dwmac.yaml#
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - nuvoton,npcm8xx-sgmii
> +          - const: snps,dwmac-3.50a

Blank line

> +  reg:
> +    items:
> +      - description:
> +          The first range represent DWMAC controller registers.

Keep it in one line and drop redundant "The first range represent".

> +      - description:
> +          The second range represent PCS configuration registers.
> +
> +  clocks:
> +    items:
> +      - description: GMAC main clock
> +
> +  clock-names:
> +    items:
> +      - const: stmmaceth
> +

Constrain also interrupts and resets.

> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/nuvoton,npcm845-clk.h>
> +
> +    ahb {
> +      #address-cells = <2>;
> +      #size-cells = <2>;

Your indentation is mixed. Use 4 spaces for example indentation.

> +
> +      gmac0: ethernet@f0802000 {

Best regards,
Krzysztof

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[kernel test robot]

Hi Tomer,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on net-next/main net/main linus/master v6.7-rc2 next-20231121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tomer-Maimon/dt-bindings-net-Add-support-NPCM-dwmac/20231121-231908
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20231121151733.2015384-3-tmaimon77%40gmail.com
patch subject: [PATCH v1 2/2] net: stmmac: Add NPCM support
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20231122/202311221011.bsAmeQ6v-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231122/202311221011.bsAmeQ6v-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311221011.bsAmeQ6v-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/ethernet/stmicro/stmmac/dwmac-npcm.c:53:6: warning: no previous prototype for function 'npcm_dwmac_pcs_init' [-Wmissing-prototypes]
   void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
        ^
   drivers/net/ethernet/stmicro/stmmac/dwmac-npcm.c:53:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
   ^
   static 
   1 warning generated.


vim +/npcm_dwmac_pcs_init +53 drivers/net/ethernet/stmicro/stmmac/dwmac-npcm.c

    52	
  > 53	void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
    54				 struct plat_stmmacenet_data *plat_dat)
    55	{
    56		u16 val;
    57	
    58		iowrite16((u16)(SR_MII_CTRL >> 9), dwmac->reg + IND_AC_BA_REG);
    59		val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
    60		val |= PCS_RST;
    61		iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
    62	
    63		while (val & PCS_RST)
    64			val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
    65	
    66		val &= ~(PCS_AN_ENABLE);
    67		iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
    68	}
    69	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[kernel test robot]

Hi Tomer,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on net-next/main net/main linus/master v6.7-rc2 next-20231121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tomer-Maimon/dt-bindings-net-Add-support-NPCM-dwmac/20231121-231908
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20231121151733.2015384-3-tmaimon77%40gmail.com
patch subject: [PATCH v1 2/2] net: stmmac: Add NPCM support
config: arc-allmodconfig (https://download.01.org/0day-ci/archive/20231122/202311221228.eaAlsztJ-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231122/202311221228.eaAlsztJ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311221228.eaAlsztJ-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/ethernet/stmicro/stmmac/dwmac-npcm.c:53:6: warning: no previous prototype for 'npcm_dwmac_pcs_init' [-Wmissing-prototypes]
      53 | void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
         |      ^~~~~~~~~~~~~~~~~~~


vim +/npcm_dwmac_pcs_init +53 drivers/net/ethernet/stmicro/stmmac/dwmac-npcm.c

    52	
  > 53	void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
    54				 struct plat_stmmacenet_data *plat_dat)
    55	{
    56		u16 val;
    57	
    58		iowrite16((u16)(SR_MII_CTRL >> 9), dwmac->reg + IND_AC_BA_REG);
    59		val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
    60		val |= PCS_RST;
    61		iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
    62	
    63		while (val & PCS_RST)
    64			val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
    65	
    66		val &= ~(PCS_AN_ENABLE);
    67		iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
    68	}
    69	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[Tomer Maimon]

Hi Russell,

Thanks for your comments.

On Tue, 21 Nov 2023 at 17:45, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Tue, Nov 21, 2023 at 05:17:33PM +0200, Tomer Maimon wrote:
> > Add Nuvoton NPCM BMC SoCs support to STMMAC dwmac driver.
> >
> > And modify MAINTAINERS to add a new F: entry for this driver.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
>
> A few comments on this...
>
> > +#define IND_AC_BA_REG                0x1FE
> > +#define SR_MII_CTRL          0x3E0000
> > +
> > +#define PCS_SR_MII_CTRL_REG  0x0
> > +#define PCS_SPEED_SELECT6    BIT(6)
> > +#define PCS_AN_ENABLE                BIT(12)
> > +#define PCS_SPEED_SELECT13   BIT(13)
> > +#define PCS_RST                      BIT(15)
>
> include/uapi/linux/mii.h:
>
> #define BMCR_SPEED1000          0x0040  /* MSB of Speed (1000)         */
> #define BMCR_ANENABLE           0x1000  /* Enable auto negotiation     */
> #define BMCR_SPEED100           0x2000  /* Select 100Mbps              */
> #define BMCR_RESET              0x8000  /* Reset to default state      */
>
> Look familiar? Maybe use the standard definitions for a standardised
> register?
>
> > +void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
> > +                      struct plat_stmmacenet_data *plat_dat)
> > +{
> > +     u16 val;
> > +
> > +     iowrite16((u16)(SR_MII_CTRL >> 9), dwmac->reg + IND_AC_BA_REG);
> > +     val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> > +     val |= PCS_RST;
> > +     iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > +
> > +     while (val & PCS_RST)
> > +             val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
>
> What if the PCS never clears its reset bit? Maybe use
> read_poll_timeout() ?
>
> > +
> > +     val &= ~(PCS_AN_ENABLE);
> > +     iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > +}
>
> Also, maybe it's time to require new stmmac platform support to start
> making use of the phylink PCS support rather than continuing to code its
> own?
>
> I notice, however, that you always disable inband signalling - please
> explain why. Also, what protocol does the PCS use when communicating
> with the PHY?
With disable inband signalling you mean disable auto negotiation? if
yes it is because the PCS sgmii is connected to the external phy AN
and is not working between the PCS and external phy.
accessing the PCS registers is indirect. The top 13 bits (bits 21-9)
of the offset have to be written to Indirect Access Base register
bits 12:0 before indirectly accessing the target register with the
offset of the bottom 9 bits (8:0) of the offset
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Thanks,

Tomer

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[Tomer Maimon]

Hi Andrew,

Thanks for your comments

On Tue, 21 Nov 2023 at 18:42, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > +void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
> > +                      struct plat_stmmacenet_data *plat_dat)
> > +{
> > +     u16 val;
> > +
> > +     iowrite16((u16)(SR_MII_CTRL >> 9), dwmac->reg + IND_AC_BA_REG);
> > +     val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> > +     val |= PCS_RST;
> > +     iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > +
> > +     while (val & PCS_RST)
> > +             val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> > +
> > +     val &= ~(PCS_AN_ENABLE);
> > +     iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > +}
>
> Is this a licensed PCS implementation? Or home grown? If its been
> licensed from somebody, it maybe should live in driver/net/pcs, so
> others can reuse it when they license the same core.
we are using DWC PCS, I don't see support for DWC PCS and I am not
sure it is supposed to be supported at /drivers/net/pcs
I do see a patch set to support DWC PCS but I don't think it answers my needs
https://patchwork.ozlabs.org/project/netdev/patch/1559674736-2190-3-git-send-email-weifeng.voon@intel.com/
>
>        Andrew

Thanks,

Tomer

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[Andrew Lunn]

On Wed, Nov 22, 2023 at 07:50:57PM +0200, Tomer Maimon wrote:
> Hi Andrew,
> 
> Thanks for your comments
> 
> On Tue, 21 Nov 2023 at 18:42, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > +void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
> > > +                      struct plat_stmmacenet_data *plat_dat)
> > > +{
> > > +     u16 val;
> > > +
> > > +     iowrite16((u16)(SR_MII_CTRL >> 9), dwmac->reg + IND_AC_BA_REG);
> > > +     val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > +     val |= PCS_RST;
> > > +     iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > +
> > > +     while (val & PCS_RST)
> > > +             val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > +
> > > +     val &= ~(PCS_AN_ENABLE);
> > > +     iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > +}
> >
> > Is this a licensed PCS implementation? Or home grown? If its been
> > licensed from somebody, it maybe should live in driver/net/pcs, so
> > others can reuse it when they license the same core.

> we are using DWC PCS, I don't see support for DWC PCS and I am not
> sure it is supposed to be supported at /drivers/net/pcs

I've not followed the naming used by Synopsys. Is DWC PCS the same as
XPCS? Does Synopsys have multiple PCS implementations?

> I do see a patch set to support DWC PCS but I don't think it answers my needs
> https://patchwork.ozlabs.org/project/netdev/patch/1559674736-2190-3-git-send-email-weifeng.voon@intel.com/

I _think_ this patch eventually got turned into
driver/net/pcs/pcs-xpcs.c

What exactly does it not do for you?

     Andrew

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[Tomer Maimon]

Hi Andrew,

On Wed, 22 Nov 2023 at 20:45, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Nov 22, 2023 at 07:50:57PM +0200, Tomer Maimon wrote:
> > Hi Andrew,
> >
> > Thanks for your comments
> >
> > On Tue, 21 Nov 2023 at 18:42, Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > > +void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
> > > > +                      struct plat_stmmacenet_data *plat_dat)
> > > > +{
> > > > +     u16 val;
> > > > +
> > > > +     iowrite16((u16)(SR_MII_CTRL >> 9), dwmac->reg + IND_AC_BA_REG);
> > > > +     val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > > +     val |= PCS_RST;
> > > > +     iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > > +
> > > > +     while (val & PCS_RST)
> > > > +             val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > > +
> > > > +     val &= ~(PCS_AN_ENABLE);
> > > > +     iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > > +}
> > >
> > > Is this a licensed PCS implementation? Or home grown? If its been
> > > licensed from somebody, it maybe should live in driver/net/pcs, so
> > > others can reuse it when they license the same core.
>
> > we are using DWC PCS, I don't see support for DWC PCS and I am not
> > sure it is supposed to be supported at /drivers/net/pcs
>
> I've not followed the naming used by Synopsys. Is DWC PCS the same as
> XPCS? Does Synopsys have multiple PCS implementations?
>
> > I do see a patch set to support DWC PCS but I don't think it answers my needs
> > https://patchwork.ozlabs.org/project/netdev/patch/1559674736-2190-3-git-send-email-weifeng.voon@intel.com/
>
> I _think_ this patch eventually got turned into
> driver/net/pcs/pcs-xpcs.c
>
> What exactly does it not do for you?
Thanks for pointing me to Synopsys (DWC) PCS in pcs-xpcs.c I need to
check if the driver follows all our SGMII needs
>
>      Andrew

Best regards,

Tomer

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[Tomer Maimon]

Hi Andrew,

I took a look at the xpcs driver and the stmmac driver and it doesn't
cover NPCM use.

in the NPCM case the stmmac ID=0x37 therefore the driver is linked to DWMAC1000
https://elixir.bootlin.com/linux/v6.7-rc2/source/drivers/net/ethernet/stmicro/stmmac/hwif.c#L139

to enable the xpcs, the stmmac should support xgmac or gmac4 and in
the NPCM is support only gmac.
https://elixir.bootlin.com/linux/v6.7-rc2/source/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c#L555
https://elixir.bootlin.com/linux/v6.7-rc2/source/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c#L573

and the most important thing is that the XPCS is handled through an
indirect register access and not through MDIO. the MDIO is connected
to the external PHY and not to the XPCS.

In that case, I think the best way to handle the XPCS is through the
NPCM glue layer, what do you think?

Thanks,

Tomer

On Thu, 23 Nov 2023 at 15:50, Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> Hi Andrew,
>
> On Wed, 22 Nov 2023 at 20:45, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Wed, Nov 22, 2023 at 07:50:57PM +0200, Tomer Maimon wrote:
> > > Hi Andrew,
> > >
> > > Thanks for your comments
> > >
> > > On Tue, 21 Nov 2023 at 18:42, Andrew Lunn <andrew@lunn.ch> wrote:
> > > >
> > > > > +void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
> > > > > +                      struct plat_stmmacenet_data *plat_dat)
> > > > > +{
> > > > > +     u16 val;
> > > > > +
> > > > > +     iowrite16((u16)(SR_MII_CTRL >> 9), dwmac->reg + IND_AC_BA_REG);
> > > > > +     val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > > > +     val |= PCS_RST;
> > > > > +     iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > > > +
> > > > > +     while (val & PCS_RST)
> > > > > +             val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > > > +
> > > > > +     val &= ~(PCS_AN_ENABLE);
> > > > > +     iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > > > +}
> > > >
> > > > Is this a licensed PCS implementation? Or home grown? If its been
> > > > licensed from somebody, it maybe should live in driver/net/pcs, so
> > > > others can reuse it when they license the same core.
> >
> > > we are using DWC PCS, I don't see support for DWC PCS and I am not
> > > sure it is supposed to be supported at /drivers/net/pcs
> >
> > I've not followed the naming used by Synopsys. Is DWC PCS the same as
> > XPCS? Does Synopsys have multiple PCS implementations?
> >
> > > I do see a patch set to support DWC PCS but I don't think it answers my needs
> > > https://patchwork.ozlabs.org/project/netdev/patch/1559674736-2190-3-git-send-email-weifeng.voon@intel.com/
> >
> > I _think_ this patch eventually got turned into
> > driver/net/pcs/pcs-xpcs.c
> >
> > What exactly does it not do for you?
> Thanks for pointing me to Synopsys (DWC) PCS in pcs-xpcs.c I need to
> check if the driver follows all our SGMII needs
> >
> >      Andrew
>
> Best regards,
>
> Tomer

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[Russell King (Oracle)]

On Wed, Nov 22, 2023 at 07:23:28PM +0200, Tomer Maimon wrote:
> Hi Russell,
> 
> Thanks for your comments.
> 
> On Tue, 21 Nov 2023 at 17:45, Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
> >
> > On Tue, Nov 21, 2023 at 05:17:33PM +0200, Tomer Maimon wrote:
> > > Add Nuvoton NPCM BMC SoCs support to STMMAC dwmac driver.
> > >
> > > And modify MAINTAINERS to add a new F: entry for this driver.
> > >
> > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> >
> > A few comments on this...
> >
> > > +#define IND_AC_BA_REG                0x1FE
> > > +#define SR_MII_CTRL          0x3E0000
> > > +
> > > +#define PCS_SR_MII_CTRL_REG  0x0
> > > +#define PCS_SPEED_SELECT6    BIT(6)
> > > +#define PCS_AN_ENABLE                BIT(12)
> > > +#define PCS_SPEED_SELECT13   BIT(13)
> > > +#define PCS_RST                      BIT(15)
> >
> > include/uapi/linux/mii.h:
> >
> > #define BMCR_SPEED1000          0x0040  /* MSB of Speed (1000)         */
> > #define BMCR_ANENABLE           0x1000  /* Enable auto negotiation     */
> > #define BMCR_SPEED100           0x2000  /* Select 100Mbps              */
> > #define BMCR_RESET              0x8000  /* Reset to default state      */
> >
> > Look familiar? Maybe use the standard definitions for a standardised
> > register?
> >
> > > +void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
> > > +                      struct plat_stmmacenet_data *plat_dat)
> > > +{
> > > +     u16 val;
> > > +
> > > +     iowrite16((u16)(SR_MII_CTRL >> 9), dwmac->reg + IND_AC_BA_REG);
> > > +     val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > +     val |= PCS_RST;
> > > +     iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > +
> > > +     while (val & PCS_RST)
> > > +             val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> >
> > What if the PCS never clears its reset bit? Maybe use
> > read_poll_timeout() ?
> >
> > > +
> > > +     val &= ~(PCS_AN_ENABLE);
> > > +     iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > +}
> >
> > Also, maybe it's time to require new stmmac platform support to start
> > making use of the phylink PCS support rather than continuing to code its
> > own?
> >
> > I notice, however, that you always disable inband signalling - please
> > explain why. Also, what protocol does the PCS use when communicating
> > with the PHY?
> With disable inband signalling you mean disable auto negotiation?

Over a SGMII, 1000base-X, USXGMII etc link, there is an inband
signalling method. Whether it is "auto negotiation" depends on your
point of view.

With 1000base-X, it is "auto negotiation" because the two link partners
advertise their abilities, and resolve the operational link parameters.
In essence, there is a negotiation between both ends.

In the case of e.g. Cisco SGMII, "auto negotiation" is a total misnomer.
There is no "negotiation". The SGMII PHY side re-purposes the 1000base-X
inband 16-bit control word to inform the MAC about the negotiated
speed and duplex settings, and the MAC can only say "yes thank you for
that" back to the PHY. There is no "and this is what I'm doing" to it.
So there's no "negotiation" in SGMII.

So, I prefer using "inband signalling" because that more accurately
describes both of these situations, whereas "auto negotiation" does
not.

Note also that whenever I see "SGMII", that means Cisco's SGMII,
which is 1000base-X modified by Cisco, and doesn't include the IEEE
802.3 1000base-X.

> if
> yes it is because the PCS sgmii is connected to the external phy AN
> and is not working between the PCS and external phy.

What if the external PHY wants to use Cisco SGMII inband signalling?

> accessing the PCS registers is indirect. The top 13 bits (bits 21-9)
> of the offset have to be written to Indirect Access Base register
> bits 12:0 before indirectly accessing the target register with the
> offset of the bottom 9 bits (8:0) of the offset

I'm not sure how this connects with my email. I asked what protocol
is used between the PCS and PHY, and I _think_ you've said that it's
Cisco SGMII.

Please give details of which PHY is being used - I'd like to know
more about why the inband signalling isn't being used.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[Andrew Lunn]

On Mon, Nov 27, 2023 at 05:19:15PM +0200, Tomer Maimon wrote:
> Hi Andrew,
> 
> I took a look at the xpcs driver and the stmmac driver and it doesn't
> cover NPCM use.
> 
> in the NPCM case the stmmac ID=0x37 therefore the driver is linked to DWMAC1000
> https://elixir.bootlin.com/linux/v6.7-rc2/source/drivers/net/ethernet/stmicro/stmmac/hwif.c#L139
> 
> to enable the xpcs, the stmmac should support xgmac or gmac4 and in
> the NPCM is support only gmac.
> https://elixir.bootlin.com/linux/v6.7-rc2/source/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c#L555
> https://elixir.bootlin.com/linux/v6.7-rc2/source/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c#L573
> 
> and the most important thing is that the XPCS is handled through an
> indirect register access and not through MDIO. the MDIO is connected
> to the external PHY and not to the XPCS.

What really matters here is, is the PCS hardware block you have an
XPCS? We don't want two drivers for the same block of hardware. 

MDIO vs indirect register access can be solved with a bit of
layering. That is not a reason to write a second driver.

	Andrew

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[Tomer Maimon]

Hi Russell,

On Mon, 27 Nov 2023 at 17:58, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Wed, Nov 22, 2023 at 07:23:28PM +0200, Tomer Maimon wrote:
> > Hi Russell,
> >
> > Thanks for your comments.
> >
> > On Tue, 21 Nov 2023 at 17:45, Russell King (Oracle)
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Tue, Nov 21, 2023 at 05:17:33PM +0200, Tomer Maimon wrote:
> > > > Add Nuvoton NPCM BMC SoCs support to STMMAC dwmac driver.
> > > >
> > > > And modify MAINTAINERS to add a new F: entry for this driver.
> > > >
> > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > >
> > > A few comments on this...
> > >
> > > > +#define IND_AC_BA_REG                0x1FE
> > > > +#define SR_MII_CTRL          0x3E0000
> > > > +
> > > > +#define PCS_SR_MII_CTRL_REG  0x0
> > > > +#define PCS_SPEED_SELECT6    BIT(6)
> > > > +#define PCS_AN_ENABLE                BIT(12)
> > > > +#define PCS_SPEED_SELECT13   BIT(13)
> > > > +#define PCS_RST                      BIT(15)
> > >
> > > include/uapi/linux/mii.h:
> > >
> > > #define BMCR_SPEED1000          0x0040  /* MSB of Speed (1000)         */
> > > #define BMCR_ANENABLE           0x1000  /* Enable auto negotiation     */
> > > #define BMCR_SPEED100           0x2000  /* Select 100Mbps              */
> > > #define BMCR_RESET              0x8000  /* Reset to default state      */
> > >
> > > Look familiar? Maybe use the standard definitions for a standardised
> > > register?
> > >
> > > > +void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
> > > > +                      struct plat_stmmacenet_data *plat_dat)
> > > > +{
> > > > +     u16 val;
> > > > +
> > > > +     iowrite16((u16)(SR_MII_CTRL >> 9), dwmac->reg + IND_AC_BA_REG);
> > > > +     val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > > +     val |= PCS_RST;
> > > > +     iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > > +
> > > > +     while (val & PCS_RST)
> > > > +             val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> > >
> > > What if the PCS never clears its reset bit? Maybe use
> > > read_poll_timeout() ?
> > >
> > > > +
> > > > +     val &= ~(PCS_AN_ENABLE);
> > > > +     iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > > > +}
> > >
> > > Also, maybe it's time to require new stmmac platform support to start
> > > making use of the phylink PCS support rather than continuing to code its
> > > own?
> > >
> > > I notice, however, that you always disable inband signalling - please
> > > explain why. Also, what protocol does the PCS use when communicating
> > > with the PHY?
> > With disable inband signalling you mean disable auto negotiation?
>
> Over a SGMII, 1000base-X, USXGMII etc link, there is an inband
> signalling method. Whether it is "auto negotiation" depends on your
> point of view.
>
> With 1000base-X, it is "auto negotiation" because the two link partners
> advertise their abilities, and resolve the operational link parameters.
> In essence, there is a negotiation between both ends.
>
> In the case of e.g. Cisco SGMII, "auto negotiation" is a total misnomer.
> There is no "negotiation". The SGMII PHY side re-purposes the 1000base-X
> inband 16-bit control word to inform the MAC about the negotiated
> speed and duplex settings, and the MAC can only say "yes thank you for
> that" back to the PHY. There is no "and this is what I'm doing" to it.
> So there's no "negotiation" in SGMII.
>
> So, I prefer using "inband signalling" because that more accurately
> describes both of these situations, whereas "auto negotiation" does
> not.
>
> Note also that whenever I see "SGMII", that means Cisco's SGMII,
> which is 1000base-X modified by Cisco, and doesn't include the IEEE
> 802.3 1000base-X.
>
> > if
> > yes it is because the PCS sgmii is connected to the external phy AN
> > and is not working between the PCS and external phy.
>
> What if the external PHY wants to use Cisco SGMII inband signalling?
This is not working with the external PHY we using (BCM54210S) and
this is why we disable the inband signalling in the PCS.
> > accessing the PCS registers is indirect. The top 13 bits (bits 21-9)
> > of the offset have to be written to Indirect Access Base register
> > bits 12:0 before indirectly accessing the target register with the
> > offset of the bottom 9 bits (8:0) of the offset
>
> I'm not sure how this connects with my email. I asked what protocol
> is used between the PCS and PHY, and I _think_ you've said that it's
> Cisco SGMII.
>
> Please give details of which PHY is being used - I'd like to know
> more about why the inband signalling isn't being used.
We are using BRCM PHY, BCM54210S.
>
> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Thanks,

Tomer

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[Tomer Maimon]

Hi Andrew,


On Wed, 29 Nov 2023 at 01:31, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Nov 27, 2023 at 05:19:15PM +0200, Tomer Maimon wrote:
> > Hi Andrew,
> >
> > I took a look at the xpcs driver and the stmmac driver and it doesn't
> > cover NPCM use.
> >
> > in the NPCM case the stmmac ID=0x37 therefore the driver is linked to DWMAC1000
> > https://elixir.bootlin.com/linux/v6.7-rc2/source/drivers/net/ethernet/stmicro/stmmac/hwif.c#L139
> >
> > to enable the xpcs, the stmmac should support xgmac or gmac4 and in
> > the NPCM is support only gmac.
> > https://elixir.bootlin.com/linux/v6.7-rc2/source/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c#L555
> > https://elixir.bootlin.com/linux/v6.7-rc2/source/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c#L573
> >
> > and the most important thing is that the XPCS is handled through an
> > indirect register access and not through MDIO. the MDIO is connected
> > to the external PHY and not to the XPCS.
>
> What really matters here is, is the PCS hardware block you have an
> XPCS? We don't want two drivers for the same block of hardware.
>
> MDIO vs indirect register access can be solved with a bit of
> layering. That is not a reason to write a second driver.
I will check with the xpcs maintainer how can we add indirect access
to the xpcs module.
>
>         Andrew

Thanks.

Tomer

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[Andrew Lunn]

> I will check with the xpcs maintainer how can we add indirect access
> to the xpcs module.

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c#L449

It creates a regmap for the memory range. On top of that it creates an
MDIO bus. You can then access the PCS in the normal way.

	Andrew

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[Tomer Maimon]

Hi Andrew,

Thanks for your input.

On Thu, 30 Nov 2023 at 19:26, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > I will check with the xpcs maintainer how can we add indirect access
> > to the xpcs module.
>
> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c#L449
>
> It creates a regmap for the memory range. On top of that it creates an
> MDIO bus. You can then access the PCS in the normal way.
>
>         Andrew

Best Regards,

Tomer

Re: [PATCH v1 2/2] net: stmmac: Add NPCM support

[Serge Semin]

On Thu, Nov 30, 2023 at 06:26:13PM +0100, Andrew Lunn wrote:
> > I will check with the xpcs maintainer how can we add indirect access
> > to the xpcs module.
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c#L449
> 
> It creates a regmap for the memory range. On top of that it creates an
> MDIO bus. You can then access the PCS in the normal way.

Actually Synopsys DW XPCS can be synthesized with two types of the CSR
interfaces:
1. MDIO: device looks as a normal MDIO device. This option is currently
   supported by the STMMAC MDIO driver.
2. MCI/APB3: device MMD CSRs are directly (all CSRs are visible) or
   indirectly (paged-base access) accessible over the system memory bus.

In addition to the above XPCS device can be equipped with separate
clock sources (at least to feed the MCI or APB3 interface) and may
have dedicated IRQ line to signal various events like link
establishing, failures, etc. From that perspective XPCS in both cases
looks as a normal platform device for which would be better to have a
special DT-node defined with all those resources supplied. Then the
XPCS DT-node could be passed to the DW MAC DT-node via the already
standardized "pcs-handle" DT-property.

I have such approach implemented in my local repo. If you consider
this as a proper solution, after a small modification I'll be able to
submit a patchset for review tomorrow.

-Serge(y)

> 
> 	Andrew
> 

Re: [Linux-stm32] [PATCH v1 2/2] net: stmmac: Add NPCM support

[Maxime Chevallier]

Hello,

On Thu, 30 Nov 2023 22:59:32 +0300
Serge Semin <fancer.lancer@gmail.com> wrote:

> On Thu, Nov 30, 2023 at 06:26:13PM +0100, Andrew Lunn wrote:
> > > I will check with the xpcs maintainer how can we add indirect access
> > > to the xpcs module.  
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c#L449
> > 
> > It creates a regmap for the memory range. On top of that it creates an
> > MDIO bus. You can then access the PCS in the normal way.  
> 
> Actually Synopsys DW XPCS can be synthesized with two types of the CSR
> interfaces:
> 1. MDIO: device looks as a normal MDIO device. This option is currently
>    supported by the STMMAC MDIO driver.
> 2. MCI/APB3: device MMD CSRs are directly (all CSRs are visible) or
>    indirectly (paged-base access) accessible over the system memory bus.
> 
> In addition to the above XPCS device can be equipped with separate
> clock sources (at least to feed the MCI or APB3 interface) and may
> have dedicated IRQ line to signal various events like link
> establishing, failures, etc. From that perspective XPCS in both cases
> looks as a normal platform device for which would be better to have a
> special DT-node defined with all those resources supplied. Then the
> XPCS DT-node could be passed to the DW MAC DT-node via the already
> standardized "pcs-handle" DT-property.

To my understanding, this should work, there's another PCS that works
this way : 
https://elixir.bootlin.com/linux/v6.7-rc3/source/drivers/net/pcs/pcs-rzn1-miic.c

Are you still able to use the mdio-regmap glue that Andrew mentioned,
to avoid the duplication between the mdio and mmio register accesses ?

Maxime

Re: [Linux-stm32] [PATCH v1 2/2] net: stmmac: Add NPCM support

[Serge Semin]

Hi Maxime

On Thu, Nov 30, 2023 at 09:34:41PM +0100, Maxime Chevallier wrote:
> Hello,
> 
> On Thu, 30 Nov 2023 22:59:32 +0300
> Serge Semin <fancer.lancer@gmail.com> wrote:
> 
> > On Thu, Nov 30, 2023 at 06:26:13PM +0100, Andrew Lunn wrote:
> > > > I will check with the xpcs maintainer how can we add indirect access
> > > > to the xpcs module.  
> > > 
> > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c#L449
> > > 
> > > It creates a regmap for the memory range. On top of that it creates an
> > > MDIO bus. You can then access the PCS in the normal way.  
> > 
> > Actually Synopsys DW XPCS can be synthesized with two types of the CSR
> > interfaces:
> > 1. MDIO: device looks as a normal MDIO device. This option is currently
> >    supported by the STMMAC MDIO driver.
> > 2. MCI/APB3: device MMD CSRs are directly (all CSRs are visible) or
> >    indirectly (paged-base access) accessible over the system memory bus.
> > 
> > In addition to the above XPCS device can be equipped with separate
> > clock sources (at least to feed the MCI or APB3 interface) and may
> > have dedicated IRQ line to signal various events like link
> > establishing, failures, etc. From that perspective XPCS in both cases
> > looks as a normal platform device for which would be better to have a
> > special DT-node defined with all those resources supplied. Then the
> > XPCS DT-node could be passed to the DW MAC DT-node via the already
> > standardized "pcs-handle" DT-property.
> 

> To my understanding, this should work, there's another PCS that works
> this way : 
> https://elixir.bootlin.com/linux/v6.7-rc3/source/drivers/net/pcs/pcs-rzn1-miic.c

It is similar to that, but since DW XPCS can reside on the normal MDIO
bus and in the system memory I took a liberty to implement the DW XPCS
MCI/APB3 interface support in the framework of the MDIO subsystem,
especially seeing Synopsys call them just "Management Interfaces", the
MMD CSRs can be indirectly accessible and since potentially there can
be more than one XPCS device on the same MCI/APB3 bus.

> 
> Are you still able to use the mdio-regmap glue that Andrew mentioned,
> to avoid the duplication between the mdio and mmio register accesses ?

Andrew cited the glue code using the Lynx PCS driver. In my case it's
DW XPCS driver. In anycase my patchset is designed in a way so not to
break (hopefully) the current DW XPCS driver users (STMMAC Eth for
sure, WangSun XGBE, SJA1105 DSA). So it will be still possible to create a
dedicated MDIO bus (using mdio-regmap API too) with the XPCS device
being detectable on it.

-Serge(y)

> 
> Maxime