[U-Boot] [PATCH 0/4] riscv: sifive: fu540: Make GEM 10/100 Mbps work

[U-Boot] [PATCH 0/4] riscv: sifive: fu540: Make GEM 10/100 Mbps work

[Bin Meng]

At present the GEM ethernet on SiFive Unleashed board can only work
in 1000 Mbps mode. With a 10/100 Mbps connection it just fails to do
any network communication.

This adds a new GEMGXL clock driver to adjust the clock settings per
the connection speed so that 10/100 Mbps works.


Bin Meng (4):
  clk: sifive: Add clock driver for GEMGXL MGMT
  dm: net: macb: Update macb_linkspd_cb() signature
  dm: net: macb: Implement link speed change callback
  sifive: fu540: Enable GEMGXL MGMT driver

 board/sifive/fu540/Kconfig       |  1 +
 drivers/clk/sifive/Kconfig       |  7 +++++
 drivers/clk/sifive/Makefile      |  2 ++
 drivers/clk/sifive/gemgxl-mgmt.c | 60 ++++++++++++++++++++++++++++++++++++++++
 drivers/net/macb.c               | 48 +++++++++++++++++++++++++++++++-
 5 files changed, 117 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/sifive/gemgxl-mgmt.c

-- 
2.7.4

[U-Boot] [PATCH 1/4] clk: sifive: Add clock driver for GEMGXL MGMT

[Bin Meng]

This adds a clock driver to support the GEMGXL management IP block
found in FU540 SoCs to control GEM TX clock operation mode for
10/100/1000 Mbps.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/clk/sifive/Kconfig       |  7 +++++
 drivers/clk/sifive/Makefile      |  2 ++
 drivers/clk/sifive/gemgxl-mgmt.c | 60 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+)
 create mode 100644 drivers/clk/sifive/gemgxl-mgmt.c

diff --git a/drivers/clk/sifive/Kconfig b/drivers/clk/sifive/Kconfig
index 81fc9f8..644881b 100644
--- a/drivers/clk/sifive/Kconfig
+++ b/drivers/clk/sifive/Kconfig
@@ -17,3 +17,10 @@ config CLK_SIFIVE_FU540_PRCI
 	  Supports the Power Reset Clock interface (PRCI) IP block found in
 	  FU540 SoCs.  If this kernel is meant to run on a SiFive FU540 SoC,
 	  enable this driver.
+
+config CLK_SIFIVE_GEMGXL_MGMT
+	bool "GEMGXL management for SiFive FU540 SoCs"
+	depends on CLK_SIFIVE
+	help
+	  Supports the GEMGXL management IP block found in FU540 SoCs to
+	  control GEM TX clock operation mode for 10/100/1000 Mbps.
diff --git a/drivers/clk/sifive/Makefile b/drivers/clk/sifive/Makefile
index 1155e07..f8263e7 100644
--- a/drivers/clk/sifive/Makefile
+++ b/drivers/clk/sifive/Makefile
@@ -3,3 +3,5 @@
 obj-$(CONFIG_CLK_ANALOGBITS_WRPLL_CLN28HPC)	+= wrpll-cln28hpc.o
 
 obj-$(CONFIG_CLK_SIFIVE_FU540_PRCI)		+= fu540-prci.o
+
+obj-$(CONFIG_CLK_SIFIVE_GEMGXL_MGMT)		+= gemgxl-mgmt.o
diff --git a/drivers/clk/sifive/gemgxl-mgmt.c b/drivers/clk/sifive/gemgxl-mgmt.c
new file mode 100644
index 0000000..d989989
--- /dev/null
+++ b/drivers/clk/sifive/gemgxl-mgmt.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019, Bin Meng <bmeng.cn@gmail.com>
+ */
+
+#include <common.h>
+#include <clk-uclass.h>
+#include <dm.h>
+#include <asm/io.h>
+
+struct gemgxl_mgmt_regs {
+	__u32 tx_clk_sel;
+};
+
+struct gemgxl_mgmt_platdata {
+	struct gemgxl_mgmt_regs *regs;
+};
+
+static int gemgxl_mgmt_ofdata_to_platdata(struct udevice *dev)
+{
+	struct gemgxl_mgmt_platdata *plat = dev_get_platdata(dev);
+
+	plat->regs = (struct gemgxl_mgmt_regs *)dev_read_addr(dev);
+
+	return 0;
+}
+
+static ulong gemgxl_mgmt_set_rate(struct clk *clk, ulong rate)
+{
+	struct gemgxl_mgmt_platdata *plat = dev_get_platdata(clk->dev);
+
+	/*
+	 * GEMGXL TX clock operation mode:
+	 *
+	 * 0 = GMII mode. Use 125 MHz gemgxlclk from PRCI in TX logic
+	 *     and output clock on GMII output signal GTX_CLK
+	 * 1 = MII mode. Use MII input signal TX_CLK in TX logic
+	 */
+	writel(rate != 125000000, &plat->regs->tx_clk_sel);
+
+	return 0;
+}
+
+const struct clk_ops gemgxl_mgmt_ops = {
+	.set_rate = gemgxl_mgmt_set_rate,
+};
+
+static const struct udevice_id gemgxl_mgmt_match[] = {
+	{ .compatible = "sifive,cadencegemgxlmgmt0", },
+	{ /* sentinel */ }
+};
+
+U_BOOT_DRIVER(gemgxl_mgmt) = {
+	.name = "gemgxl-mgmt",
+	.id = UCLASS_CLK,
+	.of_match = gemgxl_mgmt_match,
+	.ofdata_to_platdata = gemgxl_mgmt_ofdata_to_platdata,
+	.platdata_auto_alloc_size = sizeof(struct gemgxl_mgmt_platdata),
+	.ops = &gemgxl_mgmt_ops,
+};
-- 
2.7.4

[U-Boot] [PATCH 2/4] dm: net: macb: Update macb_linkspd_cb() signature

[Bin Meng]

This updates DM version macb_linkspd_cb() signature for future
expansion, eg: adding an implementation for link speed changes.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/net/macb.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index 7261416..b7f404e 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -488,15 +488,23 @@ static int macb_phy_find(struct macb_device *macb, const char *name)
 
 /**
  * macb_linkspd_cb - Linkspeed change callback function
- * @regs:	Base Register of MACB devices
+ * @dev/@regs:	MACB udevice (DM version) or
+ *		Base Register of MACB devices (non-DM version)
  * @speed:	Linkspeed
  * Returns 0 when operation success and negative errno number
  * when operation failed.
  */
+#ifdef CONFIG_DM_ETH
+int __weak macb_linkspd_cb(struct udevice *dev, unsigned int speed)
+{
+	return 0;
+}
+#else
 int __weak macb_linkspd_cb(void *regs, unsigned int speed)
 {
 	return 0;
 }
+#endif
 
 #ifdef CONFIG_DM_ETH
 static int macb_phy_init(struct udevice *dev, const char *name)
@@ -589,7 +597,11 @@ static int macb_phy_init(struct macb_device *macb, const char *name)
 
 			macb_writel(macb, NCFGR, ncfgr);
 
+#ifdef CONFIG_DM_ETH
+			ret = macb_linkspd_cb(dev, _1000BASET);
+#else
 			ret = macb_linkspd_cb(macb->regs, _1000BASET);
+#endif
 			if (ret)
 				return ret;
 
@@ -614,9 +626,17 @@ static int macb_phy_init(struct macb_device *macb, const char *name)
 	ncfgr &= ~(MACB_BIT(SPD) | MACB_BIT(FD) | GEM_BIT(GBE));
 	if (speed) {
 		ncfgr |= MACB_BIT(SPD);
+#ifdef CONFIG_DM_ETH
+		ret = macb_linkspd_cb(dev, _100BASET);
+#else
 		ret = macb_linkspd_cb(macb->regs, _100BASET);
+#endif
 	} else {
+#ifdef CONFIG_DM_ETH
+		ret = macb_linkspd_cb(dev, _10BASET);
+#else
 		ret = macb_linkspd_cb(macb->regs, _10BASET);
+#endif
 	}
 
 	if (ret)
-- 
2.7.4

[U-Boot] [PATCH 3/4] dm: net: macb: Implement link speed change callback

[Bin Meng]

At present the link speed change callback is a nop. According to
macb device tree bindings, an optional "tx_clk" is used to clock
the ethernet controller's TX_CLK under different link speed.

In 10/100 MII mode, transmit logic must be clocked from a free
running clock generated by the external PHY. In gigabit GMII mode,
the controller, not the external PHY, must generate the 125 MHz
transmit clock towards the PHY.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/net/macb.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index b7f404e..7d86fa1 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -497,6 +497,32 @@ static int macb_phy_find(struct macb_device *macb, const char *name)
 #ifdef CONFIG_DM_ETH
 int __weak macb_linkspd_cb(struct udevice *dev, unsigned int speed)
 {
+#ifdef CONFIG_CLK
+	struct clk tx_clk;
+	ulong rate;
+	int ret;
+
+	ret = clk_get_by_name(dev, "tx_clk", &tx_clk);
+	if (ret)
+		return 0;
+
+	switch (speed) {
+	case _10BASET:
+		rate = 2500000;
+		break;
+	case _100BASET:
+		rate = 25000000;
+		break;
+	case _1000BASET:
+	default:
+		rate = 125000000;
+		break;
+	}
+
+	if (tx_clk.dev)
+		clk_set_rate(&tx_clk, rate);
+#endif
+
 	return 0;
 }
 #else
-- 
2.7.4

[U-Boot] [PATCH 4/4] sifive: fu540: Enable GEMGXL MGMT driver

[Bin Meng]

Enable the new GEMGXL MGMT driver so that GEM 10/100 Mbps works now.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

 board/sifive/fu540/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/board/sifive/fu540/Kconfig b/board/sifive/fu540/Kconfig
index f464379..8eb5e30 100644
--- a/board/sifive/fu540/Kconfig
+++ b/board/sifive/fu540/Kconfig
@@ -28,6 +28,7 @@ config BOARD_SPECIFIC_OPTIONS # dummy
 	imply CMD_PING
 	imply CLK_SIFIVE
 	imply CLK_SIFIVE_FU540_PRCI
+	imply CLK_SIFIVE_GEMGXL_MGMT
 	imply DOS_PARTITION
 	imply EFI_PARTITION
 	imply IP_DYN
-- 
2.7.4

[U-Boot] [PATCH 1/4] clk: sifive: Add clock driver for GEMGXL MGMT

[Auer, Lukas]

Hi Bin,

On Thu, 2019-05-16 at 02:12 -0700, Bin Meng wrote:
> This adds a clock driver to support the GEMGXL management IP block
> found in FU540 SoCs to control GEM TX clock operation mode for
> 10/100/1000 Mbps.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  drivers/clk/sifive/Kconfig       |  7 +++++
>  drivers/clk/sifive/Makefile      |  2 ++
>  drivers/clk/sifive/gemgxl-mgmt.c | 60 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 69 insertions(+)
>  create mode 100644 drivers/clk/sifive/gemgxl-mgmt.c
> 
> diff --git a/drivers/clk/sifive/Kconfig b/drivers/clk/sifive/Kconfig
> index 81fc9f8..644881b 100644
> --- a/drivers/clk/sifive/Kconfig
> +++ b/drivers/clk/sifive/Kconfig
> @@ -17,3 +17,10 @@ config CLK_SIFIVE_FU540_PRCI
>  	  Supports the Power Reset Clock interface (PRCI) IP block found in
>  	  FU540 SoCs.  If this kernel is meant to run on a SiFive FU540 SoC,
>  	  enable this driver.
> +
> +config CLK_SIFIVE_GEMGXL_MGMT
> +	bool "GEMGXL management for SiFive FU540 SoCs"
> +	depends on CLK_SIFIVE
> +	help
> +	  Supports the GEMGXL management IP block found in FU540 SoCs to
> +	  control GEM TX clock operation mode for 10/100/1000 Mbps.
> diff --git a/drivers/clk/sifive/Makefile b/drivers/clk/sifive/Makefile
> index 1155e07..f8263e7 100644
> --- a/drivers/clk/sifive/Makefile
> +++ b/drivers/clk/sifive/Makefile
> @@ -3,3 +3,5 @@
>  obj-$(CONFIG_CLK_ANALOGBITS_WRPLL_CLN28HPC)	+= wrpll-cln28hpc.o
>  
>  obj-$(CONFIG_CLK_SIFIVE_FU540_PRCI)		+= fu540-prci.o
> +
> +obj-$(CONFIG_CLK_SIFIVE_GEMGXL_MGMT)		+= gemgxl-mgmt.o
> diff --git a/drivers/clk/sifive/gemgxl-mgmt.c b/drivers/clk/sifive/gemgxl-mgmt.c
> new file mode 100644
> index 0000000..d989989
> --- /dev/null
> +++ b/drivers/clk/sifive/gemgxl-mgmt.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019, Bin Meng <bmeng.cn@gmail.com>
> + */
> +
> +#include <common.h>
> +#include <clk-uclass.h>
> +#include <dm.h>
> +#include <asm/io.h>
> +
> +struct gemgxl_mgmt_regs {
> +	__u32 tx_clk_sel;
> +};
> +
> +struct gemgxl_mgmt_platdata {
> +	struct gemgxl_mgmt_regs *regs;
> +};
> +
> +static int gemgxl_mgmt_ofdata_to_platdata(struct udevice *dev)
> +{
> +	struct gemgxl_mgmt_platdata *plat = dev_get_platdata(dev);
> +
> +	plat->regs = (struct gemgxl_mgmt_regs *)dev_read_addr(dev);
> +
> +	return 0;
> +}
> +
> +static ulong gemgxl_mgmt_set_rate(struct clk *clk, ulong rate)
> +{
> +	struct gemgxl_mgmt_platdata *plat = dev_get_platdata(clk->dev);
> +
> +	/*
> +	 * GEMGXL TX clock operation mode:
> +	 *
> +	 * 0 = GMII mode. Use 125 MHz gemgxlclk from PRCI in TX logic
> +	 *     and output clock on GMII output signal GTX_CLK
> +	 * 1 = MII mode. Use MII input signal TX_CLK in TX logic
> +	 */
> +	writel(rate != 125000000, &plat->regs->tx_clk_sel);
> +
> +	return 0;
> +}
> +
> +const struct clk_ops gemgxl_mgmt_ops = {
> +	.set_rate = gemgxl_mgmt_set_rate,
> +};
> +
> +static const struct udevice_id gemgxl_mgmt_match[] = {
> +	{ .compatible = "sifive,cadencegemgxlmgmt0", },
> +	{ /* sentinel */ }
> +};
> +
> +U_BOOT_DRIVER(gemgxl_mgmt) = {
> +	.name = "gemgxl-mgmt",

nit: should the driver maybe be named sifive-gemgxl-mgmt to indicate
that it is a SiFive-specific driver?

Looks good otherwise!

Reviewed-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>
Tested-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>

Thanks,
Lukas

> +	.id = UCLASS_CLK,
> +	.of_match = gemgxl_mgmt_match,
> +	.ofdata_to_platdata = gemgxl_mgmt_ofdata_to_platdata,
> +	.platdata_auto_alloc_size = sizeof(struct gemgxl_mgmt_platdata),
> +	.ops = &gemgxl_mgmt_ops,
> +};

[U-Boot] [PATCH 2/4] dm: net: macb: Update macb_linkspd_cb() signature

[Auer, Lukas]

On Thu, 2019-05-16 at 02:12 -0700, Bin Meng wrote:
> This updates DM version macb_linkspd_cb() signature for future
> expansion, eg: adding an implementation for link speed changes.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  drivers/net/macb.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 

Reviewed-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>

[U-Boot] [PATCH 3/4] dm: net: macb: Implement link speed change callback

[Auer, Lukas]

Hi Bin,

On Thu, 2019-05-16 at 02:12 -0700, Bin Meng wrote:
> At present the link speed change callback is a nop. According to
> macb device tree bindings, an optional "tx_clk" is used to clock
> the ethernet controller's TX_CLK under different link speed.
> 
> In 10/100 MII mode, transmit logic must be clocked from a free
> running clock generated by the external PHY. In gigabit GMII mode,
> the controller, not the external PHY, must generate the 125 MHz
> transmit clock towards the PHY.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  drivers/net/macb.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> index b7f404e..7d86fa1 100644
> --- a/drivers/net/macb.c
> +++ b/drivers/net/macb.c
> @@ -497,6 +497,32 @@ static int macb_phy_find(struct macb_device *macb, const char *name)
>  #ifdef CONFIG_DM_ETH
>  int __weak macb_linkspd_cb(struct udevice *dev, unsigned int speed)
>  {
> +#ifdef CONFIG_CLK
> +	struct clk tx_clk;
> +	ulong rate;
> +	int ret;
> +
> +	ret = clk_get_by_name(dev, "tx_clk", &tx_clk);
> +	if (ret)
> +		return 0;

Can you return errors in ret here?

> +
> +	switch (speed) {
> +	case _10BASET:
> +		rate = 2500000;
> +		break;
> +	case _100BASET:
> +		rate = 25000000;
> +		break;
> +	case _1000BASET:
> +	default:
> +		rate = 125000000;
> +		break;

The Linux driver simply returns in the default case, without changing
tx_clk. I am wondering if we should do the same in U-Boot?

> +	}
> +
> +	if (tx_clk.dev)
> +		clk_set_rate(&tx_clk, rate);

Can you also check the return value of clk_set_rate() here?

Thanks,
Lukas

> +#endif
> +
>  	return 0;
>  }
>  #else

[U-Boot] [PATCH 4/4] sifive: fu540: Enable GEMGXL MGMT driver

[Auer, Lukas]

On Thu, 2019-05-16 at 02:12 -0700, Bin Meng wrote:
> Enable the new GEMGXL MGMT driver so that GEM 10/100 Mbps works now.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> 
> ---
> 
>  board/sifive/fu540/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 

Thanks for the patch series. I tested it successfully on a SiFive
HiFive Unleashed board at 10, 100, and 1000 Mbps link speeds. Without
the patch series, ethernet at 10 and 100 Mbps link speeds did not work.

Reviewed-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>
Tested-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>

[U-Boot] [PATCH 1/4] clk: sifive: Add clock driver for GEMGXL MGMT

[Bin Meng]

Hi Lukas,

On Mon, May 20, 2019 at 7:26 PM Auer, Lukas
<lukas.auer@aisec.fraunhofer.de> wrote:
>
> Hi Bin,
>
> On Thu, 2019-05-16 at 02:12 -0700, Bin Meng wrote:
> > This adds a clock driver to support the GEMGXL management IP block
> > found in FU540 SoCs to control GEM TX clock operation mode for
> > 10/100/1000 Mbps.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  drivers/clk/sifive/Kconfig       |  7 +++++
> >  drivers/clk/sifive/Makefile      |  2 ++
> >  drivers/clk/sifive/gemgxl-mgmt.c | 60 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 69 insertions(+)
> >  create mode 100644 drivers/clk/sifive/gemgxl-mgmt.c
> >
> > diff --git a/drivers/clk/sifive/Kconfig b/drivers/clk/sifive/Kconfig
> > index 81fc9f8..644881b 100644
> > --- a/drivers/clk/sifive/Kconfig
> > +++ b/drivers/clk/sifive/Kconfig
> > @@ -17,3 +17,10 @@ config CLK_SIFIVE_FU540_PRCI
> >         Supports the Power Reset Clock interface (PRCI) IP block found in
> >         FU540 SoCs.  If this kernel is meant to run on a SiFive FU540 SoC,
> >         enable this driver.
> > +
> > +config CLK_SIFIVE_GEMGXL_MGMT
> > +     bool "GEMGXL management for SiFive FU540 SoCs"
> > +     depends on CLK_SIFIVE
> > +     help
> > +       Supports the GEMGXL management IP block found in FU540 SoCs to
> > +       control GEM TX clock operation mode for 10/100/1000 Mbps.
> > diff --git a/drivers/clk/sifive/Makefile b/drivers/clk/sifive/Makefile
> > index 1155e07..f8263e7 100644
> > --- a/drivers/clk/sifive/Makefile
> > +++ b/drivers/clk/sifive/Makefile
> > @@ -3,3 +3,5 @@
> >  obj-$(CONFIG_CLK_ANALOGBITS_WRPLL_CLN28HPC)  += wrpll-cln28hpc.o
> >
> >  obj-$(CONFIG_CLK_SIFIVE_FU540_PRCI)          += fu540-prci.o
> > +
> > +obj-$(CONFIG_CLK_SIFIVE_GEMGXL_MGMT)         += gemgxl-mgmt.o
> > diff --git a/drivers/clk/sifive/gemgxl-mgmt.c b/drivers/clk/sifive/gemgxl-mgmt.c
> > new file mode 100644
> > index 0000000..d989989
> > --- /dev/null
> > +++ b/drivers/clk/sifive/gemgxl-mgmt.c
> > @@ -0,0 +1,60 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2019, Bin Meng <bmeng.cn@gmail.com>
> > + */
> > +
> > +#include <common.h>
> > +#include <clk-uclass.h>
> > +#include <dm.h>
> > +#include <asm/io.h>
> > +
> > +struct gemgxl_mgmt_regs {
> > +     __u32 tx_clk_sel;
> > +};
> > +
> > +struct gemgxl_mgmt_platdata {
> > +     struct gemgxl_mgmt_regs *regs;
> > +};
> > +
> > +static int gemgxl_mgmt_ofdata_to_platdata(struct udevice *dev)
> > +{
> > +     struct gemgxl_mgmt_platdata *plat = dev_get_platdata(dev);
> > +
> > +     plat->regs = (struct gemgxl_mgmt_regs *)dev_read_addr(dev);
> > +
> > +     return 0;
> > +}
> > +
> > +static ulong gemgxl_mgmt_set_rate(struct clk *clk, ulong rate)
> > +{
> > +     struct gemgxl_mgmt_platdata *plat = dev_get_platdata(clk->dev);
> > +
> > +     /*
> > +      * GEMGXL TX clock operation mode:
> > +      *
> > +      * 0 = GMII mode. Use 125 MHz gemgxlclk from PRCI in TX logic
> > +      *     and output clock on GMII output signal GTX_CLK
> > +      * 1 = MII mode. Use MII input signal TX_CLK in TX logic
> > +      */
> > +     writel(rate != 125000000, &plat->regs->tx_clk_sel);
> > +
> > +     return 0;
> > +}
> > +
> > +const struct clk_ops gemgxl_mgmt_ops = {
> > +     .set_rate = gemgxl_mgmt_set_rate,
> > +};
> > +
> > +static const struct udevice_id gemgxl_mgmt_match[] = {
> > +     { .compatible = "sifive,cadencegemgxlmgmt0", },
> > +     { /* sentinel */ }
> > +};
> > +
> > +U_BOOT_DRIVER(gemgxl_mgmt) = {
> > +     .name = "gemgxl-mgmt",
>
> nit: should the driver maybe be named sifive-gemgxl-mgmt to indicate
> that it is a SiFive-specific driver?

Will rename the driver in v2.

>
> Looks good otherwise!
>
> Reviewed-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>
> Tested-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>
>

Regards,
Bin

[U-Boot] [PATCH 3/4] dm: net: macb: Implement link speed change callback

[Bin Meng]

Hi Lukas,

On Mon, May 20, 2019 at 7:29 PM Auer, Lukas
<lukas.auer@aisec.fraunhofer.de> wrote:
>
> Hi Bin,
>
> On Thu, 2019-05-16 at 02:12 -0700, Bin Meng wrote:
> > At present the link speed change callback is a nop. According to
> > macb device tree bindings, an optional "tx_clk" is used to clock
> > the ethernet controller's TX_CLK under different link speed.
> >
> > In 10/100 MII mode, transmit logic must be clocked from a free
> > running clock generated by the external PHY. In gigabit GMII mode,
> > the controller, not the external PHY, must generate the 125 MHz
> > transmit clock towards the PHY.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  drivers/net/macb.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> > index b7f404e..7d86fa1 100644
> > --- a/drivers/net/macb.c
> > +++ b/drivers/net/macb.c
> > @@ -497,6 +497,32 @@ static int macb_phy_find(struct macb_device *macb, const char *name)
> >  #ifdef CONFIG_DM_ETH
> >  int __weak macb_linkspd_cb(struct udevice *dev, unsigned int speed)
> >  {
> > +#ifdef CONFIG_CLK
> > +     struct clk tx_clk;
> > +     ulong rate;
> > +     int ret;
> > +
> > +     ret = clk_get_by_name(dev, "tx_clk", &tx_clk);
> > +     if (ret)
> > +             return 0;
>
> Can you return errors in ret here?

No, we should ignore DT that does not contain the "tx_clk" source
hence cannot return error here. I will add a comment in v2.

>
> > +
> > +     switch (speed) {
> > +     case _10BASET:
> > +             rate = 2500000;
> > +             break;
> > +     case _100BASET:
> > +             rate = 25000000;
> > +             break;
> > +     case _1000BASET:
> > +     default:
> > +             rate = 125000000;
> > +             break;
>
> The Linux driver simply returns in the default case, without changing
> tx_clk. I am wondering if we should do the same in U-Boot?

Sure.

>
> > +     }
> > +
> > +     if (tx_clk.dev)
> > +             clk_set_rate(&tx_clk, rate);
>
> Can you also check the return value of clk_set_rate() here?

Will add the return value check in v2.

Regards,
Bin