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

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

On 6/20/19 12:55 PM, Anup Patel wrote:
>
>> -----Original Message-----
>> From: Ramon Fried <rfried.dev@gmail.com>
>> Sent: Thursday, June 20, 2019 2:48 PM
>> To: Anup Patel <Anup.Patel@wdc.com>
>> Cc: Rick Chen <rick@andestech.com>; Bin Meng <bmeng.cn@gmail.com>;
>> Lukas Auer <lukas.auer@aisec.fraunhofer.de>; Simon Glass
>> <sjg@chromium.org>; Joe Hershberger <joe.hershberger@ni.com>; Palmer
>> Dabbelt <palmer@sifive.com>; Paul Walmsley <paul.walmsley@sifive.com>;
>> Troy Benjegerdes <troy.benjegerdes@sifive.com>; Atish Patra
>> <Atish.Patra@wdc.com>; Alistair Francis <Alistair.Francis@wdc.com>; U-Boot
>> Mailing List <u-boot@lists.denx.de>
>> Subject: Re: [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.
> Sure, these are SOC specific so let me place it in
> config structure.
>
>>> +
>>> +       bool                    skip_dma_config;
>> Why do you ever need to skip_dma_config ?
> I tried all possible dma_burst_length from 0 to 16 but
> none of them worked.
>
> At the moment, with gmac_configure_dma() in-place
> this driver has stopped working on SiFive Unleashed board.
>
> Instead of skip_dma_config, I think dma_burst_length ==0 
> should be treated as skip gmac_configure_dma().

This is interesting, can you please print the dmacfg register before and after gma_configure_dma() ?

I tested this on two different boards, but maybe I missed something.


>
>>>         unsigned int            dma_burst_length;
> I was confused here. You have dma_burst_length here
> and in config structure as well. Why ?

I only had one configuration property, so I just copied it, but now that you've added more configuration items, it's reasonable to put a pointer to the config structure and use the dma_burst_length directly from there.


>
>>>         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.
> sure, I will add clock_init() callback in macb_config
>
>>> +
>>> +       /*
>>> +        * "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 ?
> Same as above comment.
>
>>>         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.
> Sure, I will add init() callback in config structure.
>
>>> +
>>>         _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)
> Well, SiFive FU540 is a SiFive SOC. I don't know how they
> have integrated MACB. May be DMA configuration for
> SiFive MACB is little different.
>
> In any case, it was working before so we should atleast
> get it back to working state by skipping DMA configuration
> when dma_burst_lenght == 0.

Like I written before, additionally, I'm writing a new patch that increases buffer size to 2K for
GEM supported devices, this will go in dma configuration, it's pitty that you will miss this improvement.
Let's try to fix gmac_configure_dma() for you.

>>> +       .has_sifive_gemgxl = true,
>> can be dropped if callback functions are declared.
> Sure.
>
>>> +};
>>> +
>>>  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.
> Regards,
> Anup