Re: [PATCH] drivers/net: Add support for 10G Base-R IP core version to xilinx_axi_enet driver

14 Dec 2020

Hi,
Thanks for sending the patch.
Please fix subject line.
I am missing commit message and your SOB line.
Also below it looks like it is not sent by git send-email.
On 08. 12. 20 1:32, Alessandro Temil wrote:
...

diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c
index 8af3711204..3b739bd407 100644
--- a/drivers/net/xilinx_axi_emac.c
+++ b/drivers/net/xilinx_axi_emac.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*


Copyright (C) 2020 Waymo LLC
Copyright (C) 2011 Michal Simek monstr@monstr.eu
Copyright (C) 2011 PetaLogix
Copyright (C) 2010 Xilinx, Inc. All rights reserved.



@@ -72,10 +73,25 @@ DECLARE_GLOBAL_DATA_PTR;
 #define XAXIDMA_BD_CTRL_TXSOF_MASK 0x08000000 /* First tx packet */
 #define XAXIDMA_BD_CTRL_TXEOF_MASK 0x04000000 /* Last tx packet */
+/* Bitmasks for the XXV Ethernet MAC */
+#define XXV_TC_TX_MASK 0x00000001
+#define XXV_TC_FCS_MASK 0x00000002
+#define XXV_RCW1_RX_MASK 0x00000001
+#define XXV_RCW1_FCS_MASK 0x00000002
Please use BIT macro instead.
...



#define DMAALIGN 128
+#define XXV_MIN_PKT_SIZE 60



static u8 rxframe[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN)));
+static u8 txminframe[XXV_MIN_PKT_SIZE] __attribute((aligned(DMAALIGN)));



+enum emac_variant {

EMAC_1G = 0,
EMAC_10G_25G,

We got recommendation to initialize all values. It means add = 1 here.
...
+};



/* Reflect dma offsets */
 struct axidma_reg {
  u32 control; /* DMACR */
@@ -97,6 +113,7 @@ struct axidma_priv {
  struct mii_dev *bus;
  u8 eth_hasnobuf;
  int phy_of_handle;

enum emac_variant mactype;

};
/* BD descriptors */
@@ -143,6 +160,14 @@ struct axi_regs {
  u32 uaw1; /* 0x704: Unicast address word 1 */
 };
+struct xxv_axi_regs {

u32 gt_reset; /* 0x0 */
u32 reserved[2];
u32 tc;       /* 0xC: Tx Configuration */
u32 reserved2;
u32 rcw1;     /* 0x14: Rx Configuration Word 1 */

+};



/* Use MII register 1 (MII status register) to detect PHY */
 #define PHY_DETECT_REG  1
@@ -191,6 +216,8 @@ static inline void axienet_dma_write(struct
axidma_bd *bd, u32 *desc)
 static u32 phyread(struct axidma_priv *priv, u32 phyaddress, u32 registernum,
     u16 *val)
 {

if (priv->mactype == EMAC_10G_25G) return 0;

Please run script/checkpatch.pl before sending the patch.
This return needs to be on next line.
And also below variables.
...

struct axi_regs *regs = priv->iobase;
u32 mdioctrlreg = 0;

@@ -217,6 +244,8 @@ static u32 phyread(struct axidma_priv *priv, u32
phyaddress, u32 registernum,
 static u32 phywrite(struct axidma_priv *priv, u32 phyaddress, u32 registernum,
      u32 data)
 {

if (priv->mactype == EMAC_10G_25G) return 0;


ditto.
...
struct axi_regs *regs = priv->iobase;
  u32 mdioctrlreg = 0;
@@ -374,6 +403,29 @@ static void axiemac_stop(struct udevice *dev)
  debug("axiemac: Halted\n");
 }
+static int xxv_axi_ethernet_init(struct axidma_priv *priv)
+{

struct xxv_axi_regs *regs = priv->iobase;

int val = readl(&regs->rcw1) & ~XXV_RCW1_FCS_MASK;

separate that variable declaration.
...

val |= XXV_RCW1_FCS_MASK;
writel(val, &regs->rcw1);

val = readl(&regs->tc) & ~XXV_TC_FCS_MASK;
val |= XXV_TC_FCS_MASK;
writel(val, &regs->tc);

val = readl(&regs->tc) & ~XXV_TC_TX_MASK;
val |= XXV_TC_TX_MASK;
writel(val, &regs->tc);

val = readl(&regs->rcw1) & ~XXV_RCW1_RX_MASK;
val |= XXV_RCW1_RX_MASK;
writel(val, &regs->rcw1);

return 0;

+}



static int axi_ethernet_init(struct axidma_priv *priv)
 {
  struct axi_regs *regs = priv->iobase;
@@ -427,6 +479,9 @@ static int axiemac_write_hwaddr(struct udevice *dev)
 {
  struct eth_pdata *pdata = dev_get_platdata(dev);
  struct axidma_priv *priv = dev_get_priv(dev);


if (priv->mactype == EMAC_10G_25G) return 0;

as above.
...

struct axi_regs *regs = priv->iobase;
/* Set the MAC address */


@@ -466,7 +521,6 @@ static void axi_dma_init(struct axidma_priv *priv)
 static int axiemac_start(struct udevice *dev)
 {
  struct axidma_priv *priv = dev_get_priv(dev);

struct axi_regs *regs = priv->iobase;
u32 temp;
debug("axiemac: Init started\n");


@@ -479,8 +533,13 @@ static int axiemac_start(struct udevice *dev)
  axi_dma_init(priv);
/* Initialize AxiEthernet hardware. */

if (axi_ethernet_init(priv))
return -1;


if (priv->mactype == EMAC_1G) {

if (axi_ethernet_init(priv))

return -1;

} else {

if (xxv_axi_ethernet_init(priv))

return -1;

}
/* Disable all RX interrupts before RxBD space setup */
temp = readl(&priv->dmarx->control);


@@ -514,15 +573,29 @@ static int axiemac_start(struct udevice *dev)
  /* Rx BD is ready - start */
  axienet_dma_write(&rx_bd, &priv->dmarx->tail);

/* Enable TX */
writel(XAE_TC_TX_MASK, &regs->tc);
/* Enable RX */
writel(XAE_RCW1_RX_MASK, &regs->rcw1);

/* PHY setup */
if (!setup_phy(dev)) {
axiemac_stop(dev);
return -1;


if (priv->mactype == EMAC_1G) {
struct axi_regs *regs = priv->iobase;
/* Enable TX */
writel(XAE_TC_TX_MASK, &regs->tc);
/* Enable RX */
writel(XAE_RCW1_RX_MASK, &regs->rcw1);

/* PHY setup */
if (!setup_phy(dev)) {
axiemac_stop(dev);
return -1;
}
} else {
struct xxv_axi_regs *regs = priv->iobase;
/* Enable TX */
temp = readl(&regs->tc) & ~XXV_TC_TX_MASK;
temp |= XXV_TC_TX_MASK;
writel(temp, &regs->tc);

/* Enable RX */
temp = readl(&regs->rcw1) & ~XXV_RCW1_RX_MASK;
temp |= XXV_RCW1_RX_MASK;
writel(temp, &regs->rcw1);
}

Please fix that coding style.
...
debug("axiemac: Init complete\n");
@@ -537,6 +610,15 @@ static int axiemac_send(struct udevice *dev, void
*ptr, int len)
  if (len > PKTSIZE_ALIGN)
  len = PKTSIZE_ALIGN;

/* If size is less than min packet size, pad to min size */
if (priv->mactype == EMAC_10G_25G
&& len < XXV_MIN_PKT_SIZE) {
memset(txminframe, 0, XXV_MIN_PKT_SIZE);
memcpy(txminframe, ptr, len);
len = XXV_MIN_PKT_SIZE;
ptr = txminframe;
}
/* Flush packet to main memory to be trasfered by DMA */
flush_cache((phys_addr_t)ptr, len);

@@ -621,7 +703,7 @@ static int axiemac_recv(struct udevice *dev, int
flags, uchar **packetp)
  temp = readl(&priv->dmarx->control);
  temp &= ~XAXIDMA_IRQ_ALL_MASK;
  writel(temp, &priv->dmarx->control);

if (!priv->eth_hasnobuf)


if ((!priv->eth_hasnobuf) && priv->mactype == EMAC_1G)
length = rx_bd.app4 & 0xFFFF; /* max length mask */
else
length = rx_bd.status & XAXIDMA_BD_STS_ACTUAL_LEN_MASK;

@@ -701,7 +783,8 @@ static int axi_emac_probe(struct udevice *dev)
  if (ret)
  return ret;

axiemac_phy_init(dev);


if (priv->mactype == EMAC_1G)
axiemac_phy_init(dev);

Maybe easier to record if phy is there in private structure. If yes do
initialization. If not skip it. It would simplify code flow.
...
return 0;
 }
@@ -736,6 +819,7 @@ static int axi_emac_ofdata_to_platdata(struct udevice *dev)
pdata->iobase = dev_read_addr(dev);
  priv->iobase = (struct axi_regs *)pdata->iobase;

   priv->mactype = dev_get_driver_data(dev);



wrong indentation.
...
offset = fdtdec_lookup_phandle(gd->fdt_blob, node,
         "axistream-connected");
@@ -760,7 +844,11 @@ static int axi_emac_ofdata_to_platdata(struct udevice *dev)
  priv->phy_of_handle = offset;
  }

phy_mode = fdt_getprop(gd->fdt_blob, node, "phy-mode", NULL);


if (priv->mactype == EMAC_1G)
phy_mode = fdt_getprop(gd->fdt_blob, node, "phy-mode", NULL);
else
phy_mode = "";

Here should be NULL. But I can't see any reason to do it in this way.
Just reduce scope of this variable and do phy setup only for 1G case and
do nothing for second.
...

if (phy_mode)
pdata->phy_interface = phy_get_interface_by_name(phy_mode);
if (pdata->phy_interface == -1) {

@@ -779,7 +867,8 @@ static int axi_emac_ofdata_to_platdata(struct udevice *dev)
 }
static const struct udevice_id axi_emac_ids[] = {

{ .compatible = "xlnx,axi-ethernet-1.00.a" },


{ .compatible = "xlnx,axi-ethernet-1.00.a", .data = (uintptr_t)EMAC_1G },
{ .compatible = "xlnx,xxv-ethernet-1.0",  .data = (uintptr_t)EMAC_10G_25G },

double space there.
...
{ }
 };
I have asked Ashok to prepare HW for it to be able to test it on HW. But
please address my comments above to be able to review it first.
Thanks,
Michal

    