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, "a))
> > + 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
