[U-Boot] [PATCH 1/2 v2] drivers: net: add NXP ENETC ethernet driver
Bin Meng
bmeng.cn at gmail.com
Wed Jun 12 05:48:44 UTC 2019
Hi Alex,
On Tue, Jun 11, 2019 at 10:27 PM Alexandru Marginean
<alexandru.marginean at 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.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.
>
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
More information about the U-Boot
mailing list