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

Anup Patel anup at brainfault.org
Tue Jun 25 05:48:37 UTC 2019


On Tue, Jun 25, 2019 at 10:51 AM Ramon Fried <rfried.dev at gmail.com> wrote:
>
> On Mon, Jun 24, 2019 at 7:03 AM Anup Patel <Anup.Patel at 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 at wdc.com>
> > ---
> >   drivers/net/macb.c | 86 +++++++++++++++++++++++++++++++++++++---------
> >   1 file changed, 69 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> > index c072f99d8f..727cb0bc49 100644
> > --- a/drivers/net/macb.c
> > +++ b/drivers/net/macb.c
> > @@ -83,7 +83,9 @@ struct macb_dma_desc {
> >
> >   struct macb_device {
> >          void                    *regs;
> > -       unsigned int            dma_burst_length;
> > +       void                    *regs1;
> This is just littering the macb_device with RISCV soc specifics,
> basically this is the SOC's
> mapping of clock tree. there's no clock tree support here, but you
> should map and use it in the
> the clock init function.
>
> > +
> > +       const struct macb_config*config;
> >
> >          unsigned int            rx_tail;
> >          unsigned int            tx_head;
> > @@ -123,6 +125,9 @@ struct macb_device {
> >
> >   struct macb_config {
> >          unsigned int            dma_burst_length;
> > +
> > +       int                     (*probe)(struct udevice *dev);
> > +       int                     (*set_tx_clk)(struct udevice *dev, ulong rate);
> Let's be more generic, let's call this function clk_init and
> initialize all the clocks there.
> A good approach will be that by default we'll call the current clock
> configuration.
> In your case you should create a new callback function that calls the
> current clock function and also
> configures the specific tx clock.

Yes, that's what we are doing. If set_tx_clk() is not available then
we fallback to current clock configuration method.

I am not attached to set_tx_clk() function name so I will just
rename it to clk_init().

> >   };
> >
> >   #ifndef CONFIG_DM_ETH
> > @@ -483,21 +488,32 @@ static int macb_phy_find(struct macb_device *macb, const char *name)
> >    * when operation failed.
> >    */
> >   #ifdef CONFIG_DM_ETH
> > +static int macb_sifive_set_tx_clk(struct udevice *dev, ulong rate)
> > +{
> > +       struct macb_device *macb = dev_get_priv(dev);
> > +
> > +       if (!macb->regs1)
> > +               return -ENODEV;
> > +
> > +       /*
> > +        * 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->regs1);
> > +       return 0;
> > +}
> > +
> >   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 +529,17 @@ int __weak macb_linkspd_cb(struct udevice *dev, unsigned int speed)
> >                  return 0;
> >          }
> >
> > +       if (macb->config->set_tx_clk)
> > +               return macb->config->set_tx_clk(dev, rate);
> > +
> > +       /*
> > +        * "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,12 +728,14 @@ static void gmac_configure_dma(struct macb_device *macb)
> >          u32 buffer_size;
> >          u32 dmacfg;
> >
> > +       if (!macb->config->dma_burst_length)
> > +               return;
> > +
> Don't skip it, we configure other things as well, including buffer size,
> which I just sent a patch that on GEM it sets it to 2K, greatly improving
> performance.

Already updated in v8 patch.

>
> >          buffer_size = 128 / RX_BUFFER_MULTIPLE;
> >          dmacfg = gem_readl(macb, DMACFG) & ~GEM_BF(RXBS, -1L);
> >          dmacfg |= GEM_BF(RXBS, buffer_size);
> >
> > -       if (macb->dma_burst_length)
> > -               dmacfg = GEM_BFINS(FBLDO, macb->dma_burst_length, dmacfg);
> > +       dmacfg = GEM_BFINS(FBLDO, macb->config->dma_burst_length, dmacfg);
> >
> >          dmacfg |= GEM_BIT(TXPBMS) | GEM_BF(RXBMS, -1L);
> >          dmacfg &= ~GEM_BIT(ENDIA_PKT);
> > @@ -1171,13 +1200,25 @@ static const struct macb_config default_gem_config = {
> >          .dma_burst_length = 16,
> >   };
> >
> > +static int macb_sifive_probe(struct udevice *dev)
> > +{
> > +       struct macb_device *macb = dev_get_priv(dev);
> > +       fdt_addr_t addr;
> > +
> > +       addr = dev_read_addr_index(dev, 1);
> > +       if (addr == FDT_ADDR_T_NONE)
> > +               return -ENODEV;
> > +       macb->regs1 = (void __iomem *)addr;
> > +
> > +       return 0;
> > +}
> Can we get rid of this and place it in the clock init function instead?

Sure, I will remove SOC specific probe and "regs1" as well.

> > +
> >   static int macb_eth_probe(struct udevice *dev)
> >   {
> > -       const struct macb_config *macb_config;
> >          struct eth_pdata *pdata = dev_get_platdata(dev);
> >          struct macb_device *macb = dev_get_priv(dev);
> >          const char *phy_mode;
> > -       __maybe_unused int ret;
> > +       int ret;
> >
> >          phy_mode = fdt_getprop(gd->fdt_blob, dev_of_offset(dev), "phy-mode",
> >                                 NULL);
> > @@ -1190,11 +1231,16 @@ static int macb_eth_probe(struct udevice *dev)
> >
> >          macb->regs = (void *)pdata->iobase;
> >
> > -       macb_config = (struct macb_config *)dev_get_driver_data(dev);
> > -       if (!macb_config)
> > -               macb_config = &default_gem_config;
> > +       macb->config = (struct macb_config *)dev_get_driver_data(dev);
> > +       if (!macb->config)
> > +               macb->config = &default_gem_config;
> > +
> > +       if (macb->config->probe) {
> > +               ret = macb->config->probe(dev);
> > +               if (ret)
> > +                       return ret;
> > +       }
> >
> > -       macb->dma_burst_length = macb_config->dma_burst_length;
> >   #ifdef CONFIG_CLK
> >          ret = macb_enable_clk(dev);
> >          if (ret)
> > @@ -1259,6 +1305,11 @@ static const struct macb_config sama5d4_config = {
> >          .dma_burst_length = 4,
> >   };
> >
> > +static const struct macb_config sifive_config = {
> > +       .probe = macb_sifive_probe,
> as stated before, probe is redundant.
> > +       .set_tx_clk = macb_sifive_set_tx_clk,
> > +};
> > +
> >   static const struct udevice_id macb_eth_ids[] = {
> >          { .compatible = "cdns,macb" },
> >          { .compatible = "cdns,at91sam9260-macb" },
> > @@ -1266,6 +1317,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
> >
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

Regards,
Anup


More information about the U-Boot mailing list