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

[Ramon Fried <rfried.dev@gmail.com>]

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.