[U-Boot] [PATCH 4/5] net: ftgmac100: add support for Aspeed SoC

Cédric Le Goater clg at kaod.org
Fri Sep 28 12:49:44 UTC 2018


Hello Simon,

On 9/27/18 3:41 PM, Simon Glass wrote:
> Hi Cedric,
> 
> On 10 September 2018 at 07:21, Cédric Le Goater <clg at kaod.org> wrote:
>> The Faraday ftgmac100 MAC controllers as found on the Aspeed SoCs have
>> some slight differences in the HW interface (End-Of-Rx/Tx-Ring
>> bits). Also include the Aspeed clock enablement.
>>
>> Signed-off-by: Cédric Le Goater <clg at kaod.org>
>> ---
>>  drivers/net/ftgmac100.h |  5 +++
>>  drivers/net/ftgmac100.c | 72 +++++++++++++++++++++++++++++++++++++----
>>  2 files changed, 71 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ftgmac100.h b/drivers/net/ftgmac100.h
>> index 9a789e4d5bee..b8f99ddf48bc 100644
>> --- a/drivers/net/ftgmac100.h
>> +++ b/drivers/net/ftgmac100.h
>> @@ -129,6 +129,11 @@ struct ftgmac100 {
>>  #define FTGMAC100_DMAFIFOS_RXDMA_REQ           BIT(30)
>>  #define FTGMAC100_DMAFIFOS_TXDMA_REQ           BIT(31)
>>
>> +/*
>> + * Feature Register
>> + */
>> +#define FTGMAC100_REVR_NEW_MDIO                BIT(31)
>> +
>>  /*
>>   * Receive buffer size register
>>   */
>> diff --git a/drivers/net/ftgmac100.c b/drivers/net/ftgmac100.c
>> index 8d7bf5b9b351..3df48a82c1ad 100644
>> --- a/drivers/net/ftgmac100.c
>> +++ b/drivers/net/ftgmac100.c
>> @@ -27,6 +27,8 @@
>>  /* PKTBUFSTX/PKTBUFSRX must both be power of 2 */
>>  #define PKTBUFSTX              4
>>
>> +#define FTGMAC100_ASPEED_NR_CLKS       2
>> +
>>  struct ftgmac100_data {
>>         phys_addr_t iobase;
>>
>> @@ -40,6 +42,11 @@ struct ftgmac100_data {
> 
> Comments on struct and members

ok.

> 
>>         struct mii_dev *bus;
>>         u32 phy_mode;
>>         u32 max_speed;
>> +
>> +       struct clk clks[FTGMAC100_ASPEED_NR_CLKS];
>> +       u32 rxdes0_edorr_mask;
>> +       u32 txdes0_edotr_mask;
>> +       bool is_aspeed;
>>  };
>>
>>  /*
>> @@ -115,9 +122,15 @@ static int ftgmac100_mdio_write(struct mii_dev *bus, int phy_addr, int dev_addr,
>>
>>  static int ftgmac100_mdio_init(struct ftgmac100_data *priv, int dev_id)
>>  {
>> +       struct ftgmac100 *ftgmac100 = (struct ftgmac100 *)priv->iobase;
>>         struct mii_dev *bus;
>>         int ret;
>>
>> +       if (priv->is_aspeed) {
> 
> Perhaps call this old_mdio_if ?

Well, this feature is related to the Aspeed socs. The old MDIO interface is used
by default so I think we don't have to force the value below. 

However, we can imagine selecting the mdio interface, new or old, through a DT 
property. I will follow your suggestion then.  
>> +               /* This driver supports the old MDIO interface */
>> +               clrbits_le32(&ftgmac100->revr, FTGMAC100_REVR_NEW_MDIO);
>> +       };
>> +
>>         bus = mdio_alloc();
>>         if (!bus)
>>                 return -ENOMEM;
>> @@ -246,13 +259,13 @@ static int ftgmac100_start(struct udevice *dev)
>>                 priv->txdes[i].txdes3 = 0;
>>                 priv->txdes[i].txdes0 = 0;
>>         }
>> -       priv->txdes[PKTBUFSTX - 1].txdes0 = FTGMAC100_TXDES0_EDOTR;
>> +       priv->txdes[PKTBUFSTX - 1].txdes0 = priv->txdes0_edotr_mask;
>>
>>         for (i = 0; i < PKTBUFSRX; i++) {
>>                 priv->rxdes[i].rxdes3 = (unsigned int)net_rx_packets[i];
>>                 priv->rxdes[i].rxdes0 = 0;
>>         }
>> -       priv->rxdes[PKTBUFSRX - 1].rxdes0 = FTGMAC100_RXDES0_EDORR;
>> +       priv->rxdes[PKTBUFSRX - 1].rxdes0 = priv->rxdes0_edorr_mask;
>>
>>         /* transmit ring */
>>         writel((u32)priv->txdes, &ftgmac100->txr_badr);
>> @@ -378,7 +391,7 @@ static int ftgmac100_send(struct udevice *dev, void *packet, int length)
>>         flush_dcache_range(data_start, data_end);
>>
>>         /* only one descriptor on TXBUF */
>> -       curr_des->txdes0 &= FTGMAC100_TXDES0_EDOTR;
>> +       curr_des->txdes0 &= priv->txdes0_edotr_mask;
>>         curr_des->txdes0 |= FTGMAC100_TXDES0_FTS |
>>                 FTGMAC100_TXDES0_LTS |
>>                 FTGMAC100_TXDES0_TXBUF_SIZE(length) |
>> @@ -409,8 +422,11 @@ static int ftgmac100_write_hwaddr(struct udevice *dev)
>>
>>  static int ftgmac100_ofdata_to_platdata(struct udevice *dev)
>>  {
>> +       struct ftgmac100_data *priv = dev_get_priv(dev);
>>         struct eth_pdata *pdata = dev_get_platdata(dev);
>>         const char *phy_mode;
>> +       int ret;
>> +       int i;
>>
>>         pdata->iobase = devfdt_get_addr(dev);
>>         pdata->phy_interface = -1;
>> @@ -424,13 +440,39 @@ static int ftgmac100_ofdata_to_platdata(struct udevice *dev)
>>
>>         pdata->max_speed = dev_read_u32_default(dev, "max-speed", 0);
>>
>> +       if (device_is_compatible(dev, "aspeed,ast2500-mac")) {
> 
> Should use dev_get_driver_data() here.

OK. I see.

> 
>> +               priv->rxdes0_edorr_mask = BIT(30);
>> +               priv->txdes0_edotr_mask = BIT(30);
>> +               priv->is_aspeed = true;
>> +       } else {
>> +               priv->rxdes0_edorr_mask = BIT(15);
>> +               priv->txdes0_edotr_mask = BIT(15);
>> +       }
>> +
>> +       if (priv->is_aspeed) {
>> +               for (i = 0; i < FTGMAC100_ASPEED_NR_CLKS; i++) {
>> +                       ret = clk_get_by_index(dev, i, &priv->clks[i]);
>> +                       if (ret) {
>> +                               dev_err(dev, "Failed to get clock: %d\n", ret);
>> +                               goto out_clk_free;
>> +                       }
>> +               }
> 
> clk_get_bulk() ?

I should help indeed. I will take a look.

> 
>> +       }
>> +
>>         return 0;
>> +
>> +out_clk_free:
>> +       while (--i >= 0)
>> +               clk_free(&priv->clks[i]);
>> +
>> +       return ret;
>>  }
>>
>>  static int ftgmac100_probe(struct udevice *dev)
>>  {
>>         struct eth_pdata *pdata = dev_get_platdata(dev);
>>         struct ftgmac100_data *priv = dev_get_priv(dev);
>> +       int i;
>>         int ret;
>>
>>         priv->iobase = pdata->iobase;
>> @@ -438,19 +480,33 @@ static int ftgmac100_probe(struct udevice *dev)
>>         priv->max_speed = pdata->max_speed;
>>         priv->phyaddr = 0;
>>
>> +       if (priv->is_aspeed) {
> 
> Why does this depend on aspeed? Can the DT specify what clocks are
> needed and how many?

yes. it does. 

I should be able to make this part more generic with clk_get_bulk() and merge 
it in the initial patch covering the whole ftgmac100 family. I only have access 
to an Aspeed one.

> 
>> +               for (i = 0; i < FTGMAC100_ASPEED_NR_CLKS; i++) {
> 
> clk_get_bulk() ?
> 
> Should use device tree to get number of clocks.
> 
>> +                       ret = clk_enable(&priv->clks[i]);
>> +                       if (ret) {
>> +                               dev_err(dev, "Failed to enable clock: %d\n",
>> +                                       ret);
>> +                               goto out_clk_release;
>> +                       }
>> +               }
>> +       }
>> +
>>         ret = ftgmac100_mdio_init(priv, dev->seq);
>>         if (ret) {
>>                 dev_err(dev, "Failed to initialize mdiobus: %d\n", ret);
>> -               goto out;
>> +               goto out_clk_release;
>>         }
>>
>>         ret = ftgmac100_phy_init(priv, dev);
>>         if (ret) {
>>                 dev_err(dev, "Failed to initialize PHY: %d\n", ret);
>> -               goto out;
>> +               goto out_clk_release;
>>         }
>>
>> -out:
>> +out_clk_release:
>> +       if (ret && priv->is_aspeed)
>> +               clk_release_all(priv->clks, FTGMAC100_ASPEED_NR_CLKS);
>> +
>>         return ret;
>>  }
>>
>> @@ -462,6 +518,9 @@ static int ftgmac100_remove(struct udevice *dev)
>>         mdio_unregister(priv->bus);
>>         mdio_free(priv->bus);
>>
>> +       if (priv->is_aspeed)
>> +               clk_release_all(priv->clks, FTGMAC100_ASPEED_NR_CLKS);
>> +
>>         return 0;
>>  }
>>
>> @@ -476,6 +535,7 @@ static const struct eth_ops ftgmac100_ops = {
>>
>>  static const struct udevice_id ftgmac100_ids[] = {
>>         { .compatible = "faraday,ftgmac100" },
>> +       { .compatible = "aspeed,ast2500-mac" },
> 
> Need .data = ASPEED here, or something like that

ok. 

Thanks for the review.

C. 

> 
>>         { }
>>  };
>>
>> --
>> 2.17.1
>>
> 
> Regards,
> Simon
> 



More information about the U-Boot mailing list