[U-Boot] [PATCH 1/2 v2] drivers: net: add NXP ENETC ethernet driver

[Alexandru Marginean]

Hi Bin,

On 6/11/2019 11:18 AM, Alexandru Marginean wrote:
> Hi Bin,
> 
> On 6/10/2019 6:42 AM, Bin Meng wrote:
>> Hi Alex,
>>
>> On Sat, Jun 8, 2019 at 12:12 AM Alex Marginean <alexm.osslist at gmail.com> wrote:
>>>
>>> Adds a driver for NXP ENETC ethernet controller currently integrated in
>>> LS1028a.  ENETC is a fairly straight-forward BD ring device and interfaces
>>> are presented as PCI EPs on the SoC ECAM.
>>>
>>> Signed-off-by: Catalin Horghidan <catalin.horghidan at nxp.com>
>>> Signed-off-by: Alex Marginean <alexm.osslist at gmail.com>
>>> ---
>>>
>>> Changes in v2:
>>>           - Added SXGMII MAC configuration
>>>           - simplified naming code in _bind
>>>           - renamed ENETC_DBG to enetc_dbg and make it use debug()
>>>           - replaced a couple of magic numbers with macros
>>>           - droped _V1 from PCI ID macro
>>>           - several styling and cosmetic updates to the header file
>>>
>>>    configs/ls1028aqds_tfa_defconfig |   1 +
>>>    configs/ls1028ardb_tfa_defconfig |   1 +
>>>    drivers/net/Kconfig              |   7 +
>>>    drivers/net/Makefile             |   1 +
>>>    drivers/net/fsl_enetc.c          | 344 +++++++++++++++++++++++++++++++
>>>    drivers/net/fsl_enetc.h          | 172 ++++++++++++++++
>>>    include/pci_ids.h                |   1 +
>>>    7 files changed, 527 insertions(+)
>>>    create mode 100644 drivers/net/fsl_enetc.c
>>>    create mode 100644 drivers/net/fsl_enetc.h
>>>

[snip]

>>> diff --git a/drivers/net/fsl_enetc.c b/drivers/net/fsl_enetc.c
>>> new file mode 100644
>>> index 0000000000..325e032746
>>> --- /dev/null
>>> +++ b/drivers/net/fsl_enetc.c
>>> @@ -0,0 +1,344 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * ENETC ethernet controller driver
>>> + * Copyright 2017-2019 NXP
>>> + */
>>> +
>>> +#include "fsl_enetc.h"
>>
>> nits: this local include should come after all other includes.
>>
>>> +
>>> +#include <dm.h>
>>> +#include <errno.h>
>>> +#include <memalign.h>
>>> +#include <asm/io.h>
>>> +#include <asm/processor.h>
>>> +#include <pci.h>
>>> +
>>> +static int enetc_bind(struct udevice *dev)
>>> +{
>>> +       char name[16];
>>> +
>>> +       sprintf(name, "enetc#%u", PCI_FUNC(dm_pci_get_bdf(dev)));
>>> +       device_set_name(dev, name);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +/*
>>> + * Probe ENETC driver:
>>> + * - initialize port and station interface BARs
>>> + */
>>> +static int enetc_probe(struct udevice *dev)
>>> +{
>>> +       struct enetc_devfn *hw = dev_get_priv(dev);
>>> +       int err = 0;
>>> +
>>> +       if (ofnode_valid(dev->node) && !ofnode_is_available(dev->node)) {
>>> +               enetc_dbg(dev, "interface disabled\n");
>>> +               return -ENODEV;
>>> +       }
>>> +
>>> +       /* initialize register */
>>> +       hw->regs_base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0, 0);
>>> +       if (!hw->regs_base) {
>>> +               enetc_dbg(dev, "failed to map BAR0\n");
>>> +               return -EINVAL;
>>> +       }
>>> +       hw->port_regs = hw->regs_base + ENETC_PORT_REGS_OFF;
>>> +
>>> +       dm_pci_clrset_config16(dev, PCI_COMMAND, 0, PCI_COMMAND_MEMORY);
>>> +
>>> +       return err;
>>> +}
>>> +
>>> +/* ENETC Port MAC address registers accept big-endian format */
>>> +static void enetc_set_primary_mac_addr(struct enetc_devfn *hw, const u8 *addr)
>>> +{
>>> +       u16 lower = *(const u16 *)(addr + 4);
>>> +       u32 upper = *(const u32 *)addr;
>>> +
>>> +       enetc_write_port(hw, ENETC_PSIPMAR0, upper);
>>> +       enetc_write_port(hw, ENETC_PSIPMAR1, lower);
>>> +}
>>> +
>>> +int enetc_enable_si_port(struct enetc_devfn *hw)
>>
>> nits: this should be static. Can we put a single line comment to
>> describe what this function does, like others?
>>
>>> +{
>>> +       u32 val;
>>> +
>>> +       /* set Rx/Tx BDR count */
>>> +       val = ENETC_PSICFGR_SET_TXBDR(ENETC_TX_BDR_CNT);
>>> +       val |= ENETC_PSICFGR_SET_RXBDR(ENETC_RX_BDR_CNT);
>>> +       enetc_write_port(hw, ENETC_PSICFGR(0), val);
>>> +       /* set Rx max frame size */
>>> +       enetc_write_port(hw, ENETC_PM_MAXFRM, ENETC_RX_MAXFRM_SIZE);
>>> +       /* enable MAC port */
>>> +       enetc_write_port(hw, ENETC_PM_CC, ENETC_PM_CC_RX_TX_EN);
>>> +       /* enable port */
>>> +       enetc_write_port(hw, ENETC_PMR, ENETC_PMR_SI0_EN);
>>> +       /* set SI cache policy */
>>> +       enetc_write(hw, ENETC_SICAR0, ENETC_SICAR_RD_CFG | ENETC_SICAR_WR_CFG);
>>> +       /* enable SI */
>>> +       enetc_write(hw, ENETC_SIMR, ENETC_SIMR_EN);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +/* Use a single set of BDs and buffers.  It's functionally OK as u-boot doesn't
>>> + * use multiple interfaces at once.
>>
>> Yep this is true for U-Boot, however it's not a good practice. Could
>> you please move these to the "struct enetc_devfn"?
>>
>>> + */
>>> +DEFINE_ALIGN_BUFFER(struct enetc_tx_bd, enetc_txbd, ENETC_BD_CNT, ENETC_ALIGN);
>>> +DEFINE_ALIGN_BUFFER(union enetc_rx_bd, enetc_rxbd, ENETC_BD_CNT, ENETC_ALIGN);
>>> +DEFINE_ALIGN_BUFFER(u8, enetc_rx_buff, ENETC_RX_MBUFF_SIZE, ENETC_ALIGN);
>>
>> For the rx buffer, isn't the one provided by U-Boot does not fit the
>> hardware requirement, so we have to create our own rx buffer?
> 
> I think the reason why we have this here is mostly related to the early
> debug days rather than HW buffer restrictions.  I'll look into removing
> the rx buffer and I'll move the descriptors to private structure.

On the buffer provided by u-boot that you mentioned, do you mean
eth_rcv_bufs?  It looks like that's only available for legacy eth.
It looks like the expectation for DM is that drivers allocate for
themselves.

Certainly each driver statically allocating for itself bloats u-boot
and  dynamic allocation in probe probably leads to fragmentation in
memory pools.  It looks to me like the eth_rcv_bufs approach is pretty
good. Is it available, or is there an alternative for DM that I missed?

If a global static like eth_rcv_bufs isn't available I'll allocate the
buffers dynamically.

Thank you!
Alex