Re: [U-Boot] [PATCH v5 6/8] net: macb: Extend MACB driver for SiFive Unleashed board

On Thu, Jun 20, 2019 at 9:49 AM Anup Patel Anup.Patel@wdc.com wrote:

The SiFive MACB ethernet has a custom TX_CLK_SEL register to select different TX clock for 1000mbps vs 10/100mbps.

This patch adds SiFive MACB compatible string and extends the MACB ethernet driver to change TX clock using TX_CLK_SEL register for SiFive MACB.

Signed-off-by: Anup Patel anup.patel@wdc.com

drivers/net/macb.c | 53 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 45 insertions(+), 8 deletions(-)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c index c072f99d8f..6a29ee3064 100644 --- a/drivers/net/macb.c +++ b/drivers/net/macb.c @@ -83,6 +83,9 @@ struct macb_dma_desc {

struct macb_device { void *regs;

•   void                    *regs_sifive_gemgxl;
  

This needs needs to be part of the specific config structure.

•   bool                    skip_dma_config;
  

Why do you ever need to skip_dma_config ?

    unsigned int            dma_burst_length;

    unsigned int            rx_tail;

@@ -122,7 +125,9 @@ struct macb_device { };

struct macb_config {

•   bool                    skip_dma_config;
    unsigned int            dma_burst_length;
  

•   bool                    has_sifive_gemgxl;
  

};

#ifndef CONFIG_DM_ETH @@ -486,18 +491,11 @@ static int macb_phy_find(struct macb_device *macb, const char *name) int __weak macb_linkspd_cb(struct udevice *dev, unsigned int speed) { #ifdef CONFIG_CLK

•   struct macb_device *macb = dev_get_priv(dev);
    struct clk tx_clk;
    ulong rate;
    int ret;
  
  

•   /*
  

•    * "tx_clk" is an optional clock source for MACB.
  

•    * Ignore if it does not exist in DT.
  

•    */
  

•   ret = clk_get_by_name(dev, "tx_clk", &tx_clk);
  

•   if (ret)
  

•           return 0;
  

•   switch (speed) {
    case _10BASET:
            rate = 2500000;         /* 2.5 MHz */
  

@@ -513,6 +511,26 @@ int __weak macb_linkspd_cb(struct udevice *dev, unsigned int speed) return 0; }

•   if (macb->regs_sifive_gemgxl) {
  

•           /*
  

•            * SiFive 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, macb->regs_sifive_gemgxl);
  

•           return 0;
  

•   }
  

move to dedicated clock init.

•   /*
  

•    * "tx_clk" is an optional clock source for MACB.
  

•    * Ignore if it does not exist in DT.
  

•    */
  

•   ret = clk_get_by_name(dev, "tx_clk", &tx_clk);
  

•   if (ret)
  

•           return 0;
  

•   if (tx_clk.dev) {
            ret = clk_set_rate(&tx_clk, rate);
            if (ret)
  

@@ -701,6 +719,9 @@ static void gmac_configure_dma(struct macb_device *macb) u32 buffer_size; u32 dmacfg;

•   if (macb->skip_dma_config)
  

•           return;
  

Same as before, why do you skip it ?

    buffer_size = 128 / RX_BUFFER_MULTIPLE;
    dmacfg = gem_readl(macb, DMACFG) & ~GEM_BF(RXBS, -1L);
    dmacfg |= GEM_BF(RXBS, buffer_size);

@@ -1178,6 +1199,7 @@ static int macb_eth_probe(struct udevice *dev) struct macb_device *macb = dev_get_priv(dev); const char *phy_mode; __maybe_unused int ret;

•   fdt_addr_t addr;
  
    phy_mode = fdt_getprop(gd->fdt_blob, dev_of_offset(dev), "phy-mode",
                           NULL);
  

@@ -1194,6 +1216,7 @@ static int macb_eth_probe(struct udevice *dev) if (!macb_config) macb_config = &default_gem_config;

•   macb->skip_dma_config = macb_config->skip_dma_config;
    macb->dma_burst_length = macb_config->dma_burst_length;
  

#ifdef CONFIG_CLK ret = macb_enable_clk(dev); @@ -1201,6 +1224,13 @@ static int macb_eth_probe(struct udevice *dev) return ret; #endif

•   if (macb_config->has_sifive_gemgxl) {
  

•           addr = dev_read_addr_index(dev, 1);
  

•           if (addr == FDT_ADDR_T_NONE)
  

•                   return -ENODEV;
  

•           macb->regs_sifive_gemgxl = (void __iomem *)addr;
  

•   }
  

This should be all done in a a specific sifive init function. You can define init function and clock init function CB functions in config (Like in Linux): " int (*clk_init)(struct platform_device *pdev, struct clk **pclk, struct clk **hclk, struct clk **tx_clk, struct clk **rx_clk); int (*init)(struct platform_device *pdev); "

and add your specific SOC initialization there. This function should be generic as possible.

•   _macb_eth_initialize(macb);
  
  

#if defined(CONFIG_CMD_MII) || defined(CONFIG_PHYLIB) @@ -1259,6 +1289,12 @@ static const struct macb_config sama5d4_config = { .dma_burst_length = 4, };

+static const struct macb_config sifive_config = {

•   .skip_dma_config = true,
  

•   .dma_burst_length = 0,
  

Don't mention it if it's zero, and by the way, I assume it should be not zero. You're wasting memory cycles, try to find the correct value. (I think 4 is the default in macb)

•   .has_sifive_gemgxl = true,
  

can be dropped if callback functions are declared.

+};

static const struct udevice_id macb_eth_ids[] = { { .compatible = "cdns,macb" }, { .compatible = "cdns,at91sam9260-macb" }, @@ -1266,6 +1302,7 @@ static const struct udevice_id macb_eth_ids[] = { { .compatible = "atmel,sama5d3-gem" }, { .compatible = "atmel,sama5d4-gem", .data = (ulong)&sama5d4_config }, { .compatible = "cdns,zynq-gem" },

•   { .compatible = "sifive,fu540-macb", .data = (ulong)&sifive_config },
    { }
  

};

-- 2.17.1

Thanks, Ramon.