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,

+};

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