Message-ID: <7cccf9f79363416ca8115a7ed9b1b7fd@sphcmbx02.sunplus.com.tw>
Date:   Thu, 25 Nov 2021 11:28:08 +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 Andrew,


Regarding hardware 'auto rmii' function of SP7021, we find a 
way to 'disable' it.

We can use 'force' mode of MAC to set link up/down, speeds, 
full/half-duplex, flow-control forcibly.

Although MAC still keeps sending MDIO commands to PHY and get 
its status, but the read-back status has no use because MAC 
is in 'force' mode.

Due to hardware design, we still need to set PHY address,
because MDIO controller of SP7021 only sends out MDIO 
commands with the same address listed in PHY address 
registers. The function below needs to be kept.

> > +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.

If it is ok, I'll modify code and send a new patch.


Best regards,
Wells


> > > +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