Hi Alex, On Tue, Jun 11, 2019 at 10:27 PM Alexandru Marginean <alexandru.margin...@nxp.com> wrote: > > 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. >
I mean net_rx_packets. > 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? > Indeed I am not sure what's the best practice for DM eth driver now. The README.drivers.eth says: "For each packet you receive, you should call the net_process_received_packet() function on it along with the packet length. The common code sets up packet buffers for you already in the .bss (net_rx_packets), so there should be no need to allocate your own." However this doc is for non-DM driver. I am hoping Joe could comment on this. > If a global static like eth_rcv_bufs isn't available I'll allocate the > buffers dynamically. > Regards, Bin _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot