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.ossl...@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.horghi...@nxp.com> >>> Signed-off-by: Alex Marginean <alexm.ossl...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot