Date:   Wed, 14 Sep 2022 04:38:23 +0000
From:   Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "geert+renesas@...der.be" <geert+renesas@...der.be>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>
Subject: RE: [PATCH 3/5] net: ethernet: renesas: Add Ethernet Switch driver

Hi Andrew,

Thank you for your review!

> From: Andrew Lunn, Sent: Tuesday, September 13, 2022 8:14 AM
> 
> > +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.

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

> > +/* 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) */

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

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

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

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

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

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

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

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

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

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

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

> > +
> > +	return -ETIMEDOUT;
> > +}
> > +
> 
> > +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

Powered by blists - more mailing lists