[U-Boot] [PATCH v3 03/13] net: ftgmac100: convert to driver model

["Cédric Le Goater" <clg@kaod.org>]

On 10/15/18 10:39 PM, Joe Hershberger wrote:
> On Wed, Oct 10, 2018 at 6:45 AM Cédric Le Goater <clg@kaod.org> wrote:
>>
>> The driver is based on the previous one and the code is only adapted
>> to fit the driver model. The support for the Faraday ftgmac100
>> controller is the same with MAC and MDIO bus support for RGMII/RMII
>> modes.
>>
>> Configuration is updated to enable compile again. At this stage, the
>> driver compiles but is not yet functional.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/netdev.h        |   1 -
>>  drivers/net/ftgmac100.c | 223 +++++++++++++++++++++++-----------------
>>  drivers/net/Kconfig     |  26 +++++
>>  3 files changed, 157 insertions(+), 93 deletions(-)
>>
>> diff --git a/include/netdev.h b/include/netdev.h
>> index 55001625fb92..0a1a3a2d8da2 100644
>> --- a/include/netdev.h
>> +++ b/include/netdev.h
>> @@ -43,7 +43,6 @@ int ethoc_initialize(u8 dev_num, int base_addr);
>>  int fec_initialize (bd_t *bis);
>>  int fecmxc_initialize(bd_t *bis);
>>  int fecmxc_initialize_multi(bd_t *bis, int dev_id, int phy_id, uint32_t addr);
>> -int ftgmac100_initialize(bd_t *bits);
>>  int ftmac100_initialize(bd_t *bits);
>>  int ftmac110_initialize(bd_t *bits);
>>  void gt6426x_eth_initialize(bd_t *bis);
>> diff --git a/drivers/net/ftgmac100.c b/drivers/net/ftgmac100.c
>> index c996f5f4a167..67a7c73503c5 100644
>> --- a/drivers/net/ftgmac100.c
>> +++ b/drivers/net/ftgmac100.c
>> @@ -7,15 +7,16 @@
>>   *
>>   * (C) Copyright 2010 Andes Technology
>>   * Macpaul Lin <macpaul@andestech.com>
>> + *
>> + * Copyright (C) 2018, IBM Corporation.
>>   */
>>
>> -#include <config.h>
>> -#include <common.h>
>> +#include <dm.h>
>> +#include <miiphy.h>
>>  #include <malloc.h>
>>  #include <net.h>
>> -#include <asm/io.h>
>> +#include <linux/io.h>
>>  #include <asm/dma-mapping.h>
>> -#include <linux/mii.h>
>>
>>  #include "ftgmac100.h"
>>
>> @@ -28,7 +29,19 @@
>>  /* PKTBUFSTX/PKTBUFSRX must both be power of 2 */
>>  #define PKTBUFSTX      4       /* must be power of 2 */
>>
>> +/**
>> + * struct ftgmac100_data - private data for the FTGMAC100 driver
>> + *
>> + * @iobase: The base address of the hardware registers
>> + * @txdes: The array of transmit descriptors
>> + * @rxdes: The array of receive descriptors
>> + * @tx_index: Transmit descriptor index in @txdes
>> + * @rx_index: Receive descriptor index in @rxdes
>> + * @phy_addr: The PHY interface address to use
>> + */
>>  struct ftgmac100_data {
>> +       struct ftgmac100 *iobase;
>> +
>>         ulong txdes_dma;
>>         struct ftgmac100_txdes *txdes;
>>         ulong rxdes_dma;
>> @@ -41,10 +54,10 @@ struct ftgmac100_data {
>>  /*
>>   * struct mii_bus functions
>>   */
>> -static int ftgmac100_mdiobus_read(struct eth_device *dev, int phy_addr,
>> -       int regnum)
>> +static int ftgmac100_mdiobus_read(struct ftgmac100_data *priv, int phy_addr,
>> +                                 int regnum)
>>  {
>> -       struct ftgmac100 *ftgmac100 = (struct ftgmac100 *)dev->iobase;
>> +       struct ftgmac100 *ftgmac100 = priv->iobase;
>>         int phycr;
>>         int i;
>>
>> @@ -76,10 +89,10 @@ static int ftgmac100_mdiobus_read(struct eth_device *dev, int phy_addr,
>>         return -1;
>>  }
>>
>> -static int ftgmac100_mdiobus_write(struct eth_device *dev, int phy_addr,
>> -       int regnum, u16 value)
>> +static int ftgmac100_mdiobus_write(struct ftgmac100_data *priv, int phy_addr,
>> +                                  int regnum, u16 value)
>>  {
>> -       struct ftgmac100 *ftgmac100 = (struct ftgmac100 *)dev->iobase;
>> +       struct ftgmac100 *ftgmac100 = priv->iobase;
>>         int phycr;
>>         int data;
>>         int i;
>> @@ -114,9 +127,10 @@ static int ftgmac100_mdiobus_write(struct eth_device *dev, int phy_addr,
>>         return -1;
>>  }
>>
>> -int ftgmac100_phy_read(struct eth_device *dev, int addr, int reg, u16 *value)
>> +int ftgmac100_phy_read(struct ftgmac100_data *priv, int addr, int reg,
>> +                      u16 *value)
>>  {
>> -       *value = ftgmac100_mdiobus_read(dev , addr, reg);
>> +       *value = ftgmac100_mdiobus_read(priv, addr, reg);
>>
>>         if (*value == -1)
>>                 return -1;
>> @@ -124,31 +138,31 @@ int ftgmac100_phy_read(struct eth_device *dev, int addr, int reg, u16 *value)
>>         return 0;
>>  }
>>
>> -int  ftgmac100_phy_write(struct eth_device *dev, int addr, int reg, u16 value)
>> +int ftgmac100_phy_write(struct ftgmac100_data *priv, int addr, int reg,
>> +                       u16 value)
>>  {
>> -       if (ftgmac100_mdiobus_write(dev, addr, reg, value) == -1)
>> +       if (ftgmac100_mdiobus_write(priv, addr, reg, value) == -1)
>>                 return -1;
>>
>>         return 0;
>>  }
>>
>> -static int ftgmac100_phy_reset(struct eth_device *dev)
>> +static int ftgmac100_phy_reset(struct ftgmac100_data *priv, struct udevice *dev)
> 
> Why does this function get a dev parameter? Seems unused.

For the printf below. later on in the pachset, the function is
completely removed, so it didn't seem important to introduce 
more changes.
 
> 
>>  {
>> -       struct ftgmac100_data *priv = dev->priv;
>>         int i;
>>         u16 status, adv;
>>
>>         adv = ADVERTISE_CSMA | ADVERTISE_ALL;
>>
>> -       ftgmac100_phy_write(dev, priv->phy_addr, MII_ADVERTISE, adv);
>> +       ftgmac100_phy_write(priv, priv->phy_addr, MII_ADVERTISE, adv);
>>
>>         printf("%s: Starting autonegotiation...\n", dev->name);
> 
> Seems like a useless print. Remove dev?>
> 
>>
>> -       ftgmac100_phy_write(dev, priv->phy_addr,
>> -               MII_BMCR, (BMCR_ANENABLE | BMCR_ANRESTART));
>> +       ftgmac100_phy_write(priv, priv->phy_addr,
>> +                           MII_BMCR, (BMCR_ANENABLE | BMCR_ANRESTART));
>>
>>         for (i = 0; i < 100000 / 100; i++) {
>> -               ftgmac100_phy_read(dev, priv->phy_addr, MII_BMSR, &status);
>> +               ftgmac100_phy_read(priv, priv->phy_addr, MII_BMSR, &status);
>>
>>                 if (status & BMSR_ANEGCOMPLETE)
>>                         break;
>> @@ -166,19 +180,17 @@ static int ftgmac100_phy_reset(struct eth_device *dev)
>>         return 1;
>>  }
>>
>> -static int ftgmac100_phy_init(struct eth_device *dev)
>> +static int ftgmac100_phy_init(struct ftgmac100_data *priv, struct udevice *dev)
> 
> Same here, why dev parameter? Only passed to reset.

The MDIO (patch 5) adds : 

    phydev = phy_connect(priv->bus, priv->phy_addr, dev, priv->phy_mode);

Nevetheless, I think we can improve the prototype of ftgmac100_phy_init()
to :

    static int ftgmac100_phy_init(struct udevice *dev) 

same for ftgmac100_mdio_init()


> 
>>  {
>> -       struct ftgmac100_data *priv = dev->priv;
>> -
>>         int phy_addr;
>>         u16 phy_id, status, adv, lpa, stat_ge;
>>         int media, speed, duplex;
>>         int i;
>>
>>         /* Check if the PHY is up to snuff... */
>> -       for (phy_addr = 0; phy_addr < CONFIG_PHY_MAX_ADDR; phy_addr++) {
>> +       for (phy_addr = 0; phy_addr < PHY_MAX_ADDR; phy_addr++) {
>>
>> -               ftgmac100_phy_read(dev, phy_addr, MII_PHYSID1, &phy_id);
>> +               ftgmac100_phy_read(priv, phy_addr, MII_PHYSID1, &phy_id);
>>
>>                 /*
>>                  * When it is unable to found PHY,
>> @@ -197,15 +209,15 @@ static int ftgmac100_phy_init(struct eth_device *dev)
>>                 return 0;
>>         }
> 
> [ ... ]
> 
> Looks good otherwise.

I will propose the prototype changes in v4.

Thanks,

C.