Hi Andrew,

Thank you for your review!

From: Andrew Lunn, Sent: Tuesday, September 13, 2022 8:14 AM

quoted

+static int default_rate = 1000;
+module_param(default_rate, int, 0644);
+MODULE_PARM_DESC(default_rate, "Default rate for both ETHA and GWCA");
+
+static int num_etha_ports = 1;
+module_param(num_etha_ports, int, 0644);
+MODULE_PARM_DESC(num_etha_ports, "Number of using ETHA ports");
+
+static int num_ndev = 1;
+module_param(num_ndev, int, 0644);
+MODULE_PARM_DESC(num_ndev, "Number of creating network devices");

No module parameters please. Find a different API for this.

I got it. I'll find a different API somehow.

quoted

+static int rswitch_reg_wait(void __iomem *addr, u32 offs, u32 mask, u32 expected)
+{
+	int i;
+
+	for (i = 0; i < RSWITCH_TIMEOUT_US; i++) {
+		if ((ioread32(addr + offs) & mask) == expected)
+			return 0;
+
+		udelay(1);
+	}
+
+	return -ETIMEDOUT;

iopoll.h

I'll use iopoll.h API.

quoted

+/* TOP */

Could you expand that acronym, for those of us how have no idea what
it means?

Sorry for lacking information. This is an abbreviation name of the block.
Complete name of that is "R-Switch-2". So, I'll modify this comment as the following:

/* R-Swtich-2 block (TOP) */

quoted

+static void rswitch_top_init(struct rswitch_private *priv)
+{
+	int i;
+
+	for (i = 0; i < RSWITCH_MAX_NUM_CHAINS; i++)
+		iowrite32((i / 16) << (GWCA_INDEX * 8), priv->addr + TPEMIMC7(i));
+}
+
+/* MFWD */

Multicast Forward?

/* Forwarding engine block (MFWD) */

# The manual doesn't mention why 'M' though....

quoted

+static void rswitch_fwd_init(struct rswitch_private *priv)
+{
+	int i;
+
+	for (i = 0; i < RSWITCH_NUM_HW; i++) {
+		iowrite32(FWPC0_DEFAULT, priv->addr + FWPC0(i));
+		iowrite32(0, priv->addr + FWPBFC(i));
+	}
+
+	for (i = 0; i < num_etha_ports; i++) {
+		iowrite32(priv->rdev[i]->rx_chain->index,
+			  priv->addr + FWPBFCSDC(GWCA_INDEX, i));
+		iowrite32(BIT(priv->gwca.index), priv->addr + FWPBFC(i));
+	}
+	iowrite32(GENMASK(num_etha_ports - 1, 0), priv->addr + FWPBFC(3));
+}
+
+/* gPTP */
+static void rswitch_get_timestamp(struct rswitch_private *priv,
+				  struct timespec64 *ts)
+{
+	struct rcar_gen4_ptp_private *ptp_priv = priv->ptp_priv;
+
+	ptp_priv->info.gettime64(&ptp_priv->info, ts);
+}
+
+/* GWCA */
+static int rswitch_gwca_change_mode(struct rswitch_private *priv,
+				    enum rswitch_gwca_mode mode)
+{
+	int ret;
+
+	if (!rswitch_agent_clock_is_enabled(priv->addr, priv->gwca.index))
+		rswitch_agent_clock_ctrl(priv->addr, priv->gwca.index, 1);
+
+	iowrite32(mode, priv->addr + GWMC);
+
+	ret = rswitch_reg_wait(priv->addr, GWMS, GWMS_OPS_MASK, mode);
+
+	if (mode == GWMC_OPC_DISABLE)
+		rswitch_agent_clock_ctrl(priv->addr, priv->gwca.index, 0);
+
+	return ret;
+}
+
+static int rswitch_gwca_mcast_table_reset(struct rswitch_private *priv)
+{
+	iowrite32(GWMTIRM_MTIOG, priv->addr + GWMTIRM);
+
+	return rswitch_reg_wait(priv->addr, GWMTIRM, GWMTIRM_MTR, GWMTIRM_MTR);
+}
+
+static int rswitch_gwca_axi_ram_reset(struct rswitch_private *priv)
+{
+	iowrite32(GWARIRM_ARIOG, priv->addr + GWARIRM);
+
+	return rswitch_reg_wait(priv->addr, GWARIRM, GWARIRM_ARR, GWARIRM_ARR);
+}
+
+static void rswitch_gwca_set_rate_limit(struct rswitch_private *priv, int rate)
+{
+	u32 gwgrlulc, gwgrlc;
+
+	switch (rate) {
+	case 1000:
+		gwgrlulc = 0x0000005f;
+		gwgrlc = 0x00010260;
+		break;
+	default:
+		dev_err(&priv->pdev->dev, "%s: This rate is not supported (%d)\n", __func__, rate);
+		break;
+	}
+
+	iowrite32(gwgrlulc, priv->addr + GWGRLULC);
+	iowrite32(gwgrlc, priv->addr + GWGRLC);

So on error, you write random values to the hardware? At least change
the break to a return.

Oops. I should use return instead. I'll fix it.

quoted

+}
+
+static bool rswitch_is_any_data_irq(struct rswitch_private *priv, u32 *dis, bool tx)
+{
+	int i;
+	u32 *mask = tx ? priv->gwca.tx_irq_bits : priv->gwca.rx_irq_bits;
+
+	for (i = 0; i < RSWITCH_NUM_IRQ_REGS; i++) {
+		if (dis[i] & mask[i])
+			return true;
+	}
+
+	return false;
+}
+
+static void rswitch_get_data_irq_status(struct rswitch_private *priv, u32 *dis)
+{
+	int i;
+
+	for (i = 0; i < RSWITCH_NUM_IRQ_REGS; i++) {
+		dis[i] = ioread32(priv->addr + GWDIS(i));
+		dis[i] &= ioread32(priv->addr + GWDIE(i));
+	}
+}
+
+static void rswitch_enadis_data_irq(struct rswitch_private *priv, int index, bool enable)
+{
+	u32 offs = enable ? GWDIE(index / 32) : GWDID(index / 32);
+
+	iowrite32(BIT(index % 32), priv->addr + offs);
+}
+
+static void rswitch_ack_data_irq(struct rswitch_private *priv, int index)
+{
+	u32 offs = GWDIS(index / 32);
+
+	iowrite32(BIT(index % 32), priv->addr + offs);
+}
+
+static bool rswitch_is_chain_rxed(struct rswitch_gwca_chain *c)
+{
+	int entry;
+	struct rswitch_ext_ts_desc *desc;
+
+	entry = c->dirty % c->num_ring;
+	desc = &c->ts_ring[entry];
+
+	if ((desc->die_dt & DT_MASK) != DT_FEMPTY)
+		return true;
+
+	return false;
+}
+
+static bool rswitch_rx(struct net_device *ndev, int *quota)
+{
+	struct rswitch_device *rdev = netdev_priv(ndev);
+	struct rswitch_gwca_chain *c = rdev->rx_chain;
+	int boguscnt = c->dirty + c->num_ring - c->cur;
+	int entry = c->cur % c->num_ring;
+	struct rswitch_ext_ts_desc *desc = &c->ts_ring[entry];
+	int limit;
+	u16 pkt_len;
+	struct sk_buff *skb;
+	dma_addr_t dma_addr;
+	u32 get_ts;

Reverse Christmas tree. Please go through all the code.

I got it. I'll fix all the code.

quoted

+
+	boguscnt = min(boguscnt, *quota);
+	limit = boguscnt;
+
+	while ((desc->die_dt & DT_MASK) != DT_FEMPTY) {
+		dma_rmb();
+		pkt_len = le16_to_cpu(desc->info_ds) & RX_DS;
+		if (--boguscnt < 0)
+			break;
+		skb = c->skb[entry];
+		c->skb[entry] = NULL;
+		dma_addr = le32_to_cpu(desc->dptrl) | ((__le64)le32_to_cpu(desc->dptrh) << 32);
+		dma_unmap_single(ndev->dev.parent, dma_addr, PKT_BUF_SZ, DMA_FROM_DEVICE);
+		get_ts = rdev->priv->ptp_priv->tstamp_rx_ctrl & RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT;
+		if (get_ts) {
+			struct skb_shared_hwtstamps *shhwtstamps;
+			struct timespec64 ts;
+
+			shhwtstamps = skb_hwtstamps(skb);
+			memset(shhwtstamps, 0, sizeof(*shhwtstamps));
+			ts.tv_sec = (u64)le32_to_cpu(desc->ts_sec);
+			ts.tv_nsec = le32_to_cpu(desc->ts_nsec & 0x3FFFFFFF);
+			shhwtstamps->hwtstamp = timespec64_to_ktime(ts);
+		}
+		skb_put(skb, pkt_len);
+		skb->protocol = eth_type_trans(skb, ndev);
+		netif_receive_skb(skb);
+		rdev->ndev->stats.rx_packets++;
+		rdev->ndev->stats.rx_bytes += pkt_len;
+
+		entry = (++c->cur) % c->num_ring;
+		desc = &c->ts_ring[entry];
+	}
+
+	/* Refill the RX ring buffers */
+	for (; c->cur - c->dirty > 0; c->dirty++) {
+		entry = c->dirty % c->num_ring;
+		desc = &c->ts_ring[entry];
+		desc->info_ds = cpu_to_le16(PKT_BUF_SZ);
+
+		if (!c->skb[entry]) {
+			skb = dev_alloc_skb(PKT_BUF_SZ + RSWITCH_ALIGN - 1);
+			if (!skb)
+				break;	/* Better luck next round */
+			skb_reserve(skb, NET_IP_ALIGN);
+			dma_addr = dma_map_single(ndev->dev.parent, skb->data,
+						  le16_to_cpu(desc->info_ds),
+						  DMA_FROM_DEVICE);
+			if (dma_mapping_error(ndev->dev.parent, dma_addr))
+				desc->info_ds = cpu_to_le16(0);
+			desc->dptrl = cpu_to_le32(lower_32_bits(dma_addr));
+			desc->dptrh = cpu_to_le32(upper_32_bits(dma_addr));

If there has been a dma mapping error, is dma_addr valid?

No, the dma_addr is not valid. So, I'll fix error handling around this.

quoted

+			skb_checksum_none_assert(skb);
+			c->skb[entry] = skb;
+		}
+		dma_wmb();
+		desc->die_dt = DT_FEMPTY | DIE;
+	}
+
+	*quota -= limit - (++boguscnt);
+
+	return boguscnt <= 0;
+}
+
+static int rswitch_tx_free(struct net_device *ndev, bool free_txed_only)
+{
+	struct rswitch_device *rdev = netdev_priv(ndev);
+	struct rswitch_ext_desc *desc;
+	int free_num = 0;
+	int entry, size;
+	dma_addr_t dma_addr;
+	struct rswitch_gwca_chain *c = rdev->tx_chain;
+	struct sk_buff *skb;
+
+	for (; c->cur - c->dirty > 0; c->dirty++) {
+		entry = c->dirty % c->num_ring;
+		desc = &c->ring[entry];
+		if (free_txed_only && (desc->die_dt & DT_MASK) != DT_FEMPTY)
+			break;
+
+		dma_rmb();
+		size = le16_to_cpu(desc->info_ds) & TX_DS;
+		skb = c->skb[entry];
+		if (skb) {
+			if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
+				struct skb_shared_hwtstamps shhwtstamps;
+				struct timespec64 ts;
+
+				rswitch_get_timestamp(rdev->priv, &ts);
+				memset(&shhwtstamps, 0, sizeof(shhwtstamps));
+				shhwtstamps.hwtstamp = timespec64_to_ktime(ts);
+				skb_tstamp_tx(skb, &shhwtstamps);
+			}
+			dma_addr = le32_to_cpu(desc->dptrl) |
+				   ((__le64)le32_to_cpu(desc->dptrh) << 32);
+			dma_unmap_single(ndev->dev.parent, dma_addr,
+					 size, DMA_TO_DEVICE);
+			dev_kfree_skb_any(c->skb[entry]);
+			c->skb[entry] = NULL;
+			free_num++;
+		}
+		desc->die_dt = DT_EEMPTY;
+		rdev->ndev->stats.tx_packets++;
+		rdev->ndev->stats.tx_bytes += size;
+	}
+
+	return free_num;
+}
+
+static int rswitch_poll(struct napi_struct *napi, int budget)
+{
+	struct net_device *ndev = napi->dev;
+	struct rswitch_device *rdev = netdev_priv(ndev);
+	struct rswitch_private *priv = rdev->priv;
+	int quota = budget;
+
+retry:
+	rswitch_tx_free(ndev, true);
+
+	if (rswitch_rx(ndev, &quota))
+		goto out;
+	else if (rswitch_is_chain_rxed(rdev->rx_chain))
+		goto retry;
+
+	netif_wake_subqueue(ndev, 0);
+
+	napi_complete(napi);
+
+	rswitch_enadis_data_irq(priv, rdev->tx_chain->index, true);
+	rswitch_enadis_data_irq(priv, rdev->rx_chain->index, true);
+
+out:
+	return budget - quota;
+}
+
+static void rswitch_queue_interrupt(struct net_device *ndev)
+{
+	struct rswitch_device *rdev = netdev_priv(ndev);
+
+	if (napi_schedule_prep(&rdev->napi)) {
+		rswitch_enadis_data_irq(rdev->priv, rdev->tx_chain->index, false);
+		rswitch_enadis_data_irq(rdev->priv, rdev->rx_chain->index, false);
+		__napi_schedule(&rdev->napi);
+	}
+}
+
+static irqreturn_t rswitch_data_irq(struct rswitch_private *priv, u32 *dis)
+{
+	struct rswitch_gwca_chain *c;
+	int i;
+	int index, bit;
+
+	for (i = 0; i < priv->gwca.num_chains; i++) {
+		c = &priv->gwca.chains[i];
+		index = c->index / 32;
+		bit = BIT(c->index % 32);
+		if (!(dis[index] & bit))
+			continue;
+
+		rswitch_ack_data_irq(priv, c->index);
+		rswitch_queue_interrupt(c->ndev);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t rswitch_gwca_irq(int irq, void *dev_id)
+{
+	struct rswitch_private *priv = dev_id;
+	irqreturn_t ret = IRQ_NONE;
+	u32 dis[RSWITCH_NUM_IRQ_REGS];
+
+	rswitch_get_data_irq_status(priv, dis);
+
+	if (rswitch_is_any_data_irq(priv, dis, true) ||
+	    rswitch_is_any_data_irq(priv, dis, false))
+		ret = rswitch_data_irq(priv, dis);
+
+	return ret;
+}
+
+static int rswitch_gwca_request_irqs(struct rswitch_private *priv)
+{
+	int i, ret;
+	char *resource_name, *irq_name;
+	struct rswitch_gwca *gwca = &priv->gwca;
+
+	for (i = 0; i < GWCA_NUM_IRQS; i++) {
+		resource_name = kasprintf(GFP_KERNEL, GWCA_IRQ_RESOURCE_NAME, i);
+		if (!resource_name) {
+			ret = -ENOMEM;
+			goto err;
+		}
+
+		gwca->irq[i] = platform_get_irq_byname(priv->pdev, resource_name);
+		kfree(resource_name);
+		if (gwca->irq[i] < 0) {
+			ret = gwca->irq[i];
+			goto err;
+		}
+
+		irq_name = devm_kasprintf(&priv->pdev->dev, GFP_KERNEL,
+					  GWCA_IRQ_NAME, i);
+		if (!irq_name) {
+			ret = -ENOMEM;
+			goto err;
+		}
+
+		ret = request_irq(gwca->irq[i], rswitch_gwca_irq, 0, irq_name, priv);

devm_request_irq() ?

Yes, I'll fix it.

quoted

+static int rswitch_etha_set_access(struct rswitch_etha *etha, bool read,
+				   int phyad, int devad, int regad, int data)
+{
+	int pop = read ? MDIO_READ_C45 : MDIO_WRITE_C45;
+	u32 val;
+	int ret;
+
+	/* No match device */
+	if (devad == 0xffffffff)
+		return 0;
+
+	writel(MMIS1_CLEAR_FLAGS, etha->addr + MMIS1);
+
+	val = MPSM_PSME | MPSM_MFF_C45;
+	iowrite32((regad << 16) | (devad << 8) | (phyad << 3) | val, etha->addr + MPSM);
+
+	ret = rswitch_reg_wait(etha->addr, MMIS1, MMIS1_PAACS, MMIS1_PAACS);
+	if (ret)
+		return ret;
+
+	rswitch_modify(etha->addr, MMIS1, MMIS1_PAACS, MMIS1_PAACS);
+
+	if (read) {
+		writel((pop << 13) | (devad << 8) | (phyad << 3) | val, etha->addr + MPSM);
+
+		ret = rswitch_reg_wait(etha->addr, MMIS1, MMIS1_PRACS, MMIS1_PRACS);
+		if (ret)
+			return ret;
+
+		ret = (ioread32(etha->addr + MPSM) & MPSM_PRD_MASK) >> 16;
+
+		rswitch_modify(etha->addr, MMIS1, MMIS1_PRACS, MMIS1_PRACS);
+	} else {
+		iowrite32((data << 16) | (pop << 13) | (devad << 8) | (phyad << 3) | val,
+			  etha->addr + MPSM);
+
+		ret = rswitch_reg_wait(etha->addr, MMIS1, MMIS1_PWACS, MMIS1_PWACS);
+	}
+
+	return ret;
+}
+
+static int rswitch_etha_mii_read(struct mii_bus *bus, int addr, int regnum)
+{
+	struct rswitch_etha *etha = bus->priv;
+	int mode, devad, regad;
+
+	mode = regnum & MII_ADDR_C45;
+	devad = (regnum >> MII_DEVADDR_C45_SHIFT) & 0x1f;
+	regad = regnum & MII_REGADDR_C45_MASK;
+
+	/* Not support Clause 22 access method */
+	if (!mode)
+		return 0;

-EOPNOTSUPP.

I'll fix it.

quoted

+
+	return rswitch_etha_set_access(etha, true, addr, devad, regad, 0);
+}
+
+static int rswitch_etha_mii_write(struct mii_bus *bus, int addr, int regnum, u16 val)
+{
+	struct rswitch_etha *etha = bus->priv;
+	int mode, devad, regad;
+
+	mode = regnum & MII_ADDR_C45;
+	devad = (regnum >> MII_DEVADDR_C45_SHIFT) & 0x1f;
+	regad = regnum & MII_REGADDR_C45_MASK;
+
+	/* Not support Clause 22 access method */
+	if (!mode)
+		return 0;

Same here.

I'll fix it.

quoted

+
+	return rswitch_etha_set_access(etha, false, addr, devad, regad, val);
+}
+
+/* Call of_node_put(port) after done */
+static struct device_node *rswitch_get_port_node(struct rswitch_device *rdev)
+{
+	struct device_node *ports, *port;
+	int err = 0;
+	u32 index;
+
+	ports = of_get_child_by_name(rdev->ndev->dev.parent->of_node, "ports");
+	if (!ports)
+		return NULL;
+
+	for_each_child_of_node(ports, port) {
+		err = of_property_read_u32(port, "reg", &index);
+		if (err < 0) {
+			port = NULL;
+			goto out;
+		}
+		if (index == rdev->etha->index)
+			break;
+	}
+
+out:
+	of_node_put(ports);
+
+	return port;
+}
+
+/* Call of_node_put(phy) after done */
+static struct device_node *rswitch_get_phy_node(struct rswitch_device *rdev)
+{
+	struct device_node *port, *phy = NULL;
+	int err = 0;
+
+	port = rswitch_get_port_node(rdev);
+	if (!port)
+		return NULL;
+
+	err = of_get_phy_mode(port, &rdev->etha->phy_interface);
+	if (err < 0)
+		goto out;
+
+	phy = of_parse_phandle(port, "phy-handle", 0);
+
+out:
+	of_node_put(port);
+
+	return phy;
+}
+
+static int rswitch_mii_register(struct rswitch_device *rdev)
+{
+	struct mii_bus *mii_bus;
+	struct device_node *port;
+	int err;
+
+	mii_bus = mdiobus_alloc();
+	if (!mii_bus)
+		return -ENOMEM;
+
+	mii_bus->name = "rswitch_mii";
+	sprintf(mii_bus->id, "etha%d", rdev->etha->index);
+	mii_bus->priv = rdev->etha;
+	mii_bus->read = rswitch_etha_mii_read;
+	mii_bus->write = rswitch_etha_mii_write;
+	mii_bus->parent = &rdev->ndev->dev;
+
+	port = rswitch_get_port_node(rdev);
+	err = of_mdiobus_register(mii_bus, port);
+	if (err < 0) {
+		mdiobus_free(mii_bus);
+		goto out;
+	}
+
+	rdev->etha->mii = mii_bus;
+
+out:
+	of_node_put(port);
+
+	return err;
+}
+
+static void rswitch_mii_unregister(struct rswitch_device *rdev)
+{
+	if (rdev->etha && rdev->etha->mii) {
+		mdiobus_unregister(rdev->etha->mii);
+		mdiobus_free(rdev->etha->mii);
+		rdev->etha->mii = NULL;
+	}
+}
+
+static void rswitch_adjust_link(struct net_device *ndev)
+{
+	struct rswitch_device *rdev = netdev_priv(ndev);
+	struct phy_device *phydev = ndev->phydev;
+
+	if (phydev->link != rdev->etha->link) {
+		phy_print_status(phydev);
+		rdev->etha->link = phydev->link;
+	}

Given that the SERDES supports 100 and 1G, it seems odd you don't need
to do anything here.

Indeed. However, unfortunately, the current hardware cannot change the speed at runtime...
So, I'll add such comments here.

quoted

+}
+
+static int rswitch_phy_init(struct rswitch_device *rdev, struct device_node *phy)
+{
+	struct phy_device *phydev;
+	int err = 0;
+
+	phydev = of_phy_connect(rdev->ndev, phy, rswitch_adjust_link, 0,
+				rdev->etha->phy_interface);
+	if (!phydev) {
+		err = -ENOENT;
+		goto out;
+	}
+
+	phy_attached_info(phydev);
+
+out:
+	return err;
+}
+
+static void rswitch_phy_deinit(struct rswitch_device *rdev)
+{
+	if (rdev->ndev->phydev) {
+		phy_disconnect(rdev->ndev->phydev);
+		rdev->ndev->phydev = NULL;
+	}
+}
+
+static int rswitch_open(struct net_device *ndev)
+{
+	struct rswitch_device *rdev = netdev_priv(ndev);
+	struct device_node *phy;
+	int err = 0;
+
+	if (rdev->etha) {
+		if (!rdev->etha->operated) {
+			phy = rswitch_get_phy_node(rdev);
+			if (!phy)
+				return -EINVAL;
+			err = rswitch_etha_hw_init(rdev->etha, ndev->dev_addr);
+			if (err < 0)
+				goto err_hw_init;
+			err = rswitch_mii_register(rdev);
+			if (err < 0)
+				goto err_mii_register;

Each port has its own MDIO bus? Not one bus shared by all ports? That
is unusual.

Each port has its own MDIO bus.

quoted

+			err = rswitch_phy_init(rdev, phy);
+			if (err < 0)
+				goto err_phy_init;
+		}
+
+		phy_start(ndev->phydev);
+
+		if (!rdev->etha->operated) {
+			err = rswitch_serdes_init(rdev->etha->serdes_addr,
+						  rdev->etha->serdes_addr0,
+						  rdev->etha->phy_interface,
+						  rdev->etha->speed);
+			if (err < 0)
+				goto err_serdes_init;
+			of_node_put(phy);
+		}
+
+		rdev->etha->operated = true;
+	}
+
+	napi_enable(&rdev->napi);
+	netif_start_queue(ndev);
+
+	rswitch_enadis_data_irq(rdev->priv, rdev->tx_chain->index, true);
+	rswitch_enadis_data_irq(rdev->priv, rdev->rx_chain->index, true);
+
+	return err;
+
+err_serdes_init:
+	phy_stop(ndev->phydev);
+	rswitch_phy_deinit(rdev);
+
+err_phy_init:
+	rswitch_mii_unregister(rdev);
+
+err_mii_register:
+err_hw_init:
+	of_node_put(phy);
+
+	return err;
+};
+
+static int rswitch_stop(struct net_device *ndev)
+{
+	struct rswitch_device *rdev = netdev_priv(ndev);
+
+	netif_tx_stop_all_queues(ndev);
+
+	rswitch_enadis_data_irq(rdev->priv, rdev->tx_chain->index, false);
+	rswitch_enadis_data_irq(rdev->priv, rdev->rx_chain->index, false);
+
+	if (rdev->etha && ndev->phydev)
+		phy_stop(ndev->phydev);
+
+	napi_disable(&rdev->napi);
+
+	return 0;
+};
+
+static int rswitch_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct rswitch_device *rdev = netdev_priv(ndev);
+	int ret = NETDEV_TX_OK;
+	int entry;
+	dma_addr_t dma_addr;
+	struct rswitch_ext_desc *desc;
+	struct rswitch_gwca_chain *c = rdev->tx_chain;
+
+	if (c->cur - c->dirty > c->num_ring - 1) {
+		netif_stop_subqueue(ndev, 0);
+		return ret;
+	}
+
+	if (skb_put_padto(skb, ETH_ZLEN))
+		return ret;
+
+	dma_addr = dma_map_single(ndev->dev.parent, skb->data, skb->len, DMA_TO_DEVICE);
+	if (dma_mapping_error(ndev->dev.parent, dma_addr)) {
+		dev_kfree_skb_any(skb);
+		return ret;
+	}
+
+	entry = c->cur % c->num_ring;
+	c->skb[entry] = skb;
+	desc = &c->ring[entry];
+	desc->dptrl = cpu_to_le32(lower_32_bits(dma_addr));
+	desc->dptrh = cpu_to_le32(upper_32_bits(dma_addr));
+	desc->info_ds = cpu_to_le16(skb->len);
+
+	if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
+		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+		rdev->ts_tag++;
+		desc->info1 = (rdev->ts_tag << 8) | BIT(3);
+	}
+
+	skb_tx_timestamp(skb);
+	dma_wmb();
+
+	desc->die_dt = DT_FSINGLE | DIE;
+	wmb();	/* c->cur must be incremented after die_dt was set */
+
+	c->cur++;
+	rswitch_modify(rdev->addr, GWTRC(c->index), 0, BIT(c->index % 32));
+
+	return ret;
+}
+
+static struct net_device_stats *rswitch_get_stats(struct net_device *ndev)
+{
+	return &ndev->stats;
+}
+
+static int rswitch_hwstamp_get(struct net_device *ndev, struct ifreq *req)
+{
+	struct rswitch_device *rdev = netdev_priv(ndev);
+	struct rswitch_private *priv = rdev->priv;
+	struct rcar_gen4_ptp_private *ptp_priv = priv->ptp_priv;
+	struct hwtstamp_config config;
+
+	config.flags = 0;
+	config.tx_type = ptp_priv->tstamp_tx_ctrl ? HWTSTAMP_TX_ON :
+						    HWTSTAMP_TX_OFF;
+	switch (ptp_priv->tstamp_rx_ctrl & RCAR_GEN4_RXTSTAMP_TYPE) {
+	case RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT:
+		config.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
+		break;
+	case RCAR_GEN4_RXTSTAMP_TYPE_ALL:
+		config.rx_filter = HWTSTAMP_FILTER_ALL;
+		break;
+	default:
+		config.rx_filter = HWTSTAMP_FILTER_NONE;
+		break;
+	}
+
+	return copy_to_user(req->ifr_data, &config, sizeof(config)) ? -EFAULT : 0;
+}
+
+static int rswitch_hwstamp_set(struct net_device *ndev, struct ifreq *req)
+{
+	struct rswitch_device *rdev = netdev_priv(ndev);
+	struct rswitch_private *priv = rdev->priv;
+	struct rcar_gen4_ptp_private *ptp_priv = priv->ptp_priv;
+	struct hwtstamp_config config;
+	u32 tstamp_rx_ctrl = RCAR_GEN4_RXTSTAMP_ENABLED;
+	u32 tstamp_tx_ctrl;
+
+	if (copy_from_user(&config, req->ifr_data, sizeof(config)))
+		return -EFAULT;
+
+	if (config.flags)
+		return -EINVAL;
+
+	switch (config.tx_type) {
+	case HWTSTAMP_TX_OFF:
+		tstamp_tx_ctrl = 0;
+		break;
+	case HWTSTAMP_TX_ON:
+		tstamp_tx_ctrl = RCAR_GEN4_TXTSTAMP_ENABLED;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	switch (config.rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		tstamp_rx_ctrl = 0;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+		tstamp_rx_ctrl |= RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT;
+		break;
+	default:
+		config.rx_filter = HWTSTAMP_FILTER_ALL;
+		tstamp_rx_ctrl |= RCAR_GEN4_RXTSTAMP_TYPE_ALL;
+		break;
+	}
+
+	ptp_priv->tstamp_tx_ctrl = tstamp_tx_ctrl;
+	ptp_priv->tstamp_rx_ctrl = tstamp_rx_ctrl;
+
+	return copy_to_user(req->ifr_data, &config, sizeof(config)) ? -EFAULT : 0;
+}
+
+static int rswitch_eth_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
+{
+	if (!netif_running(ndev))
+		return -EINVAL;
+
+	switch (cmd) {
+	case SIOCGHWTSTAMP:
+		return rswitch_hwstamp_get(ndev, req);
+	case SIOCSHWTSTAMP:
+		return rswitch_hwstamp_set(ndev, req);
+	default:
+		break;
+	}

You should call phy_mii_ioctl() here.

Thank you. I'll call phy_mii_ioctl() here.

quoted

+static int rswitch_serdes_reg_wait(void __iomem *addr, u32 offs, u32 bank, u32 mask, u32 expected)
+{
+	int i;
+
+	iowrite32(bank, addr + RSWITCH_SERDES_BANK_SELECT);
+	mdelay(1);
+
+	for (i = 0; i < RSWITCH_TIMEOUT_US; i++) {
+		if ((ioread32(addr + offs) & mask) == expected)
+			return 0;
+		udelay(1);
+	}

iopoll.h

I'll use iopoll.h API.

quoted

+
+	return -ETIMEDOUT;
+}
+

quoted

+static int rswitch_serdes_common_setting(void __iomem *addr0,
+					 enum rswitch_serdes_mode mode)
+{
+	switch (mode) {
+	case SGMII:
+		rswitch_serdes_write32(addr0, 0x0244, 0x180, 0x97);
+		rswitch_serdes_write32(addr0, 0x01d0, 0x180, 0x60);
+		rswitch_serdes_write32(addr0, 0x01d8, 0x180, 0x2200);
+		rswitch_serdes_write32(addr0, 0x01d4, 0x180, 0);
+		rswitch_serdes_write32(addr0, 0x01e0, 0x180, 0x3d);

Please add #defines for all these magic numbers.

I should have added comments before though, the datasheet also describes
such magic numbers like below...
Step S.4.1	bank 0x180	address = 0x0244		data = 0x00000097
Step S.4.2	bank 0x180	address = 0x01d0		data = 0x00000060
...

So, perhaps we can define like the followings:
#define	SERDES_BANK_180		0x180

#define	SERDES_STEP_S_4_1_ADDR	0x0244
#define	SERDES_STEP_S_4_1_DATA	0x00000097
...

And,

		rswitch_serdes_write32(addr0, SERDES_STEP_S_4_1_ADDR, SERDES_BANK_180, SERDES_STEP_S_4_1_DATA);
		rswitch_serdes_write32(addr0, SERDES_STEP_S_4_2_ADDR, SERDES_BANK_180, SERDES_STEP_S_4_2_DATA);
...

Is it acceptable?
Or, perhaps, adding the reason (The datasheet describes magic numbers)
is better than adding the #defines.

Best regards,
Yoshihiro Shimoda