Message-ID: <452b9aa57d034bed988a685d320906c6@sphcmbx02.sunplus.com.tw>
Date:   Tue, 16 Nov 2021 17:09:45 +0000
From:   Wells Lu 呂芳騰 <wells.lu@...plus.com>
To:     Andrew Lunn <andrew@...n.ch>, Wells Lu <wellslutw@...il.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        Vincent Shih 施錕鴻 <vincent.shih@...plus.com>
Subject: RE: [PATCH v2 2/2] net: ethernet: Add driver for Sunplus SP7021

Hi,


> > +void rx_descs_flush(struct sp_common *comm)
> 
> As both Florian and I have said, you need a prefix for all your functions, structures,
> etc. sp_ is not the best prefix either, it is not very unique. spl2sw_ would be better.

I'll add prefix spl2sw for all functions, structures, file-names in next patch.

I thought comment for revising prefix is only for structures, function and file name
with prefix l2sw_ because 'l2sw_' has been used by other modules.

Now I know prefix is necessary for all in this driver, except local variables and 
structure members.


> > +void rx_descs_clean(struct sp_common *comm) {
> > +	u32 i, j;
> > +	struct mac_desc *rx_desc;
> > +	struct skb_info *rx_skbinfo;
> 
> netdev wants reverse christmas tree. You need to change the order of your local variables,
> longest lines first, shorted last.

Yes, I'll rearrange local variables to 'reverse Christmas tree' order in next patch.


> > +
> > +	for (i = 0; i < RX_DESC_QUEUE_NUM; i++) {
> > +		if (!comm->rx_skb_info[i])
> > +			continue;
> > +
> > +		rx_desc = comm->rx_desc[i];
> > +		rx_skbinfo = comm->rx_skb_info[i];
> > +		for (j = 0; j < comm->rx_desc_num[i]; j++) {
> > +			rx_desc[j].cmd1 = 0;
> > +			wmb();	// Clear OWN_BIT and then set other fields.
> > +			rx_desc[j].cmd2 = 0;
> > +			rx_desc[j].addr1 = 0;
> > +
> > +			if (rx_skbinfo[j].skb) {
> > +				dma_unmap_single(&comm->pdev->dev, rx_skbinfo[j].mapping,
> > +						 comm->rx_desc_buff_size, DMA_FROM_DEVICE);
> > +				dev_kfree_skb(rx_skbinfo[j].skb);
> > +				rx_skbinfo[j].skb = NULL;
> > +				rx_skbinfo[j].mapping = 0;
> > +			}
> > +		}
> > +
> > +		kfree(rx_skbinfo);
> > +		comm->rx_skb_info[i] = NULL;
> > +	}
> > +}
> 
> > +int rx_descs_init(struct sp_common *comm) {
> > +	struct sk_buff *skb;
> > +	u32 i, j;
> > +	struct mac_desc *rx_desc;
> > +	struct skb_info *rx_skbinfo;
> > +
> > +	for (i = 0; i < RX_DESC_QUEUE_NUM; i++) {
> > +		comm->rx_skb_info[i] = kmalloc_array(comm->rx_desc_num[i],
> > +						     sizeof(struct skb_info), GFP_KERNEL);
> > +		if (!comm->rx_skb_info[i])
> > +			goto MEM_ALLOC_FAIL;
> > +
> > +		rx_skbinfo = comm->rx_skb_info[i];
> > +		rx_desc = comm->rx_desc[i];
> > +		for (j = 0; j < comm->rx_desc_num[i]; j++) {
> > +			skb = __dev_alloc_skb(comm->rx_desc_buff_size + RX_OFFSET,
> > +					      GFP_ATOMIC | GFP_DMA);
> > +			if (!skb)
> > +				goto MEM_ALLOC_FAIL;
> > +
> > +			skb->dev = comm->ndev;
> > +			skb_reserve(skb, RX_OFFSET);	/* +data +tail */
> > +
> > +			rx_skbinfo[j].skb = skb;
> > +			rx_skbinfo[j].mapping = dma_map_single(&comm->pdev->dev, skb->data,
> > +							       comm->rx_desc_buff_size,
> > +							       DMA_FROM_DEVICE);
> > +			rx_desc[j].addr1 = rx_skbinfo[j].mapping;
> > +			rx_desc[j].addr2 = 0;
> > +			rx_desc[j].cmd2 = (j == comm->rx_desc_num[i] - 1) ?
> > +					  EOR_BIT | comm->rx_desc_buff_size :
> > +					  comm->rx_desc_buff_size;
> > +			wmb();	// Set OWN_BIT after other fields are effective.
> > +			rx_desc[j].cmd1 = OWN_BIT;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +
> > +MEM_ALLOC_FAIL:
> 
> lower case labels. Didn't somebody already say that?

I'll modify all labels to lowercase in next patch.
Yes, Denis said that but patch not yet sent out.


> > +int descs_init(struct sp_common *comm) {
> > +	u32 i, ret;
> > +
> > +	// Initialize rx descriptor's data
> > +	comm->rx_desc_num[0] = RX_QUEUE0_DESC_NUM; #if RX_DESC_QUEUE_NUM > 1
> > +	comm->rx_desc_num[1] = RX_QUEUE1_DESC_NUM; #endif
> 
> Avoid #if statements. Why is this needed?

Yes, I'll remove the #if statement in next patch.
It is indeed not necessary.
RX_DESC_QUEUE_NUM is equal to 2.


> > +++ b/drivers/net/ethernet/sunplus/sp_driver.c
> > @@ -0,0 +1,606 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright Sunplus Technology Co., Ltd.
> > + *       All rights reserved.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/reset.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/of_net.h>
> > +#include "sp_driver.h"
> > +#include "sp_phy.h"
> > +
> > +static const char def_mac_addr[ETHERNET_MAC_ADDR_LEN] = {
> > +	0xfc, 0x4b, 0xbc, 0x00, 0x00, 0x00
> 
> This does not have the locally administered bit set. Should it? Or is this and address
> from your OUI?

This is default MAC address when MAC address in NVMEM is not found.
Fc:4b:bc:00:00:00 is OUI of "Sunplus Technology Co., Ltd.".
Can I keep this? or it should be removed?


> > +static void ethernet_set_rx_mode(struct net_device *ndev) {
> > +	if (ndev) {
> 
> How can ndev be NULL?

Yes, I'll remove 'if (ndev) {' statement in next patch.
It is redundant.


> > +++ b/drivers/net/ethernet/sunplus/sp_hal.c
> > @@ -0,0 +1,331 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright Sunplus Technology Co., Ltd.
> > + *       All rights reserved.
> > + */
> > +
> > +#include <linux/iopoll.h>
> > +#include "sp_hal.h"
> > +
> > +void hal_mac_stop(struct sp_mac *mac)
> 
> I suggest you avoid any references to hal. It makes people think you have ported a driver
> from some other operating system and then put a layer of code on top of it. That is not
> how you do it in Linux. This is a Linux driver, nothing else.


Yes, I'll change file name 'sp_hal.c' to 'spl2sw_hw.c'.
Function name in this file will also be changed, for example:
hal_mac_stop() --> spl2sw_hw_mac_stop()


> > +void hal_mac_reset(struct sp_mac *mac) { }
> > +
> 
> Should not exist.

Yes, I'll remove it in next patch.


> > +void hal_mac_addr_set(struct sp_mac *mac) {
> > +	struct sp_common *comm = mac->comm;
> > +	u32 reg;
> > +
> > +	// Write MAC address.
> > +	writel(mac->mac_addr[0] + (mac->mac_addr[1] << 8),
> > +	       comm->sp_reg_base + SP_W_MAC_15_0);
> > +	writel(mac->mac_addr[2] + (mac->mac_addr[3] << 8) + (mac->mac_addr[4] << 16) +
> > +	      (mac->mac_addr[5] << 24),	comm->sp_reg_base + SP_W_MAC_47_16);
> > +
> > +	// Set aging=1
> > +	writel((mac->cpu_port << 10) + (mac->vlan_id << 7) + (1 << 4) + 0x1,
> > +	       comm->sp_reg_base + SP_WT_MAC_AD0);
> 
> Is this actually adding an entry into the address translation table?
> If so, make this clear in the function name. You are not setting the MAC address, you are
> just adding a static forwarding entry.

Yes, this is actually adding an entry into address table.
I'll change function name to spl2sw_mac_add_addr() in next patch.
Is the name ok?


> > +
> > +	// Wait for completing.
> > +	do {
> > +		reg = readl(comm->sp_reg_base + SP_WT_MAC_AD0);
> > +		ndelay(10);
> > +		netdev_dbg(mac->ndev, "wt_mac_ad0 = %08x\n", reg);
> > +	} while ((reg & (0x1 << 1)) == 0x0);
> > +
> > +	netdev_dbg(mac->ndev, "mac_ad0 = %08x, mac_ad = %08x%04x\n",
> > +		   readl(comm->sp_reg_base + SP_WT_MAC_AD0),
> > +		   readl(comm->sp_reg_base + SP_W_MAC_47_16),
> > +		   readl(comm->sp_reg_base + SP_W_MAC_15_0) & 0xffff); }
> 
> > +void hal_rx_mode_set(struct net_device *ndev) {
> > +	struct sp_mac *mac = netdev_priv(ndev);
> > +	struct sp_common *comm = mac->comm;
> > +	u32 mask, reg, rx_mode;
> > +
> > +	netdev_dbg(ndev, "ndev->flags = %08x\n", ndev->flags);
> > +
> > +	mask = (mac->lan_port << 2) | (mac->lan_port << 0);
> > +	reg = readl(comm->sp_reg_base + SP_CPU_CNTL);
> > +
> > +	if (ndev->flags & IFF_PROMISC) {	/* Set promiscuous mode */
> > +		// Allow MC and unknown UC packets
> > +		rx_mode = (mac->lan_port << 2) | (mac->lan_port << 0);
> > +	} else if ((!netdev_mc_empty(ndev) && (ndev->flags & IFF_MULTICAST)) ||
> > +		   (ndev->flags & IFF_ALLMULTI)) {
> > +		// Allow MC packets
> > +		rx_mode = (mac->lan_port << 2);
> > +	} else {
> > +		// Disable MC and unknown UC packets
> > +		rx_mode = 0;
> > +	}
> > +
> > +	writel((reg & (~mask)) | ((~rx_mode) & mask), comm->sp_reg_base + SP_CPU_CNTL);
> > +	netdev_dbg(ndev, "cpu_cntl = %08x\n", readl(comm->sp_reg_base +
> > +SP_CPU_CNTL));
> 
> This looks like it belongs in the ethtool code.

This function sets receiving mode.


> > +int hal_mdio_access(struct sp_mac *mac, u8 op_cd, u8 phy_addr, u8
> > +reg_addr, u32 wdata) {
> > +	struct sp_common *comm = mac->comm;
> > +	u32 val, ret;
> > +
> > +	writel((wdata << 16) | (op_cd << 13) | (reg_addr << 8) | phy_addr,
> > +	       comm->sp_reg_base + SP_PHY_CNTL_REG0);
> > +
> > +	ret = read_poll_timeout(readl, val, val & op_cd, 10, 1000, 1,
> > +				comm->sp_reg_base + SP_PHY_CNTL_REG1);
> > +	if (ret == 0)
> > +		return val >> 16;
> > +	else
> > +		return ret;
> > +}
> 
> Should go with the other mdio code.

I'll move it into 'spl2sw_mdio.c' in next patch.

I put all hardware-related functions in sp_hal.c (will be changed to spl2sw_hw.c).
All functions in other files won't touch hardware registers.
This seems not Linux driver style.


> > +void hal_phy_addr(struct sp_mac *mac) {
> > +	struct sp_common *comm = mac->comm;
> > +	u32 reg;
> > +
> > +	// Set address of phy.
> > +	reg = readl(comm->sp_reg_base + SP_MAC_FORCE_MODE);
> > +	reg = (reg & (~(0x1f << 16))) | ((mac->phy_addr & 0x1f) << 16);
> > +	if (mac->next_ndev) {
> > +		struct net_device *ndev2 = mac->next_ndev;
> > +		struct sp_mac *mac2 = netdev_priv(ndev2);
> > +
> > +		reg = (reg & (~(0x1f << 24))) | ((mac2->phy_addr & 0x1f) << 24);
> > +	}
> > +	writel(reg, comm->sp_reg_base + SP_MAC_FORCE_MODE); }
> 
> As i said before, the hardware never directly communicates with the PHY. So you can remove
> this.

I'll remove this function in next patch.

But now I cannot find a way to disable hardware 'auto rmii' function.
If I remove this function right now, MAC may get wrong status of PHY 
from wrong address because SP7021 MAC communicates with PHY 
automatically. This may cause more problem.

I am consulting with ASIC engineer. Hopefully, someone can find 
a way to disable the auto function.


> > +static void port_status_change(struct sp_mac *mac) {
> > +	u32 reg;
> > +	struct net_device *ndev = mac->ndev;
> > +
> > +	reg = read_port_ability(mac);
> > +	if (!netif_carrier_ok(ndev) && (reg & PORT_ABILITY_LINK_ST_P0)) {
> > +		netif_carrier_on(ndev);
> 
> phylib should be handling the carrier for you.

I'll remove this function in next patch.
If 'auto rmii' function is removed, we no more need this function.


> > +	if (mac->next_ndev) {
> > +		struct net_device *ndev2 = mac->next_ndev;
> > +
> > +		if (!netif_carrier_ok(ndev2) && (reg & PORT_ABILITY_LINK_ST_P1)) {
> > +			netif_carrier_on(ndev2);
> > +			netif_start_queue(ndev2);
> > +		} else if (netif_carrier_ok(ndev2) && !(reg & PORT_ABILITY_LINK_ST_P1)) {
> > +			netif_carrier_off(ndev2);
> > +			netif_stop_queue(ndev2);
> > +		}
> 
> Looks very odd. The two netdev should be independent.

I don't understand your comment.
ndev checks PORT_ABILITY_LINK_ST_P0
ndev2 checks PORT_ABILITY_LINK_ST_P1
They are independent already.


> > diff --git a/drivers/net/ethernet/sunplus/sp_mdio.c
> > b/drivers/net/ethernet/sunplus/sp_mdio.c
> > new file mode 100644
> > index 0000000..f6a7e64
> > --- /dev/null
> > +++ b/drivers/net/ethernet/sunplus/sp_mdio.c
> > @@ -0,0 +1,90 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright Sunplus Technology Co., Ltd.
> > + *       All rights reserved.
> > + */
> > +
> > +#include "sp_mdio.h"
> > +
> > +u32 mdio_read(struct sp_mac *mac, u32 phy_id, u16 regnum) {
> > +	int ret;
> > +
> > +	ret = hal_mdio_access(mac, MDIO_READ_CMD, phy_id, regnum, 0);
> > +	if (ret < 0)
> > +		return -EOPNOTSUPP;
> > +
> > +	return ret;
> > +}
> > +
> > +u32 mdio_write(struct sp_mac *mac, u32 phy_id, u32 regnum, u16 val) {
> > +	int ret;
> > +
> > +	ret = hal_mdio_access(mac, MDIO_WRITE_CMD, phy_id, regnum, val);
> > +	if (ret < 0)
> > +		return -EOPNOTSUPP;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mii_read(struct mii_bus *bus, int phy_id, int regnum) {
> > +	struct sp_mac *mac = bus->priv;
> 
> What happened about my request to return -EOPNOTSUPP for C45 requests?

Sorry for overlooking the comment!
I am not sure how to check C45 request. Should I add statements like:

	if (regnum & MII_ADDR_C45)
		Return -EOPNOTSUPP;

for mdio_read() and mdio_write()?


>      Andrew

Thank you very much for your review!

Best regards,
Wells

Powered by blists - more mailing lists