| Message ID | 20230424053233.2338782-1-danishanwar@ti.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce ICSSG based ethernet Driver | expand |
On Mon, Apr 24, 2023 at 11:02:33AM +0530, MD Danish Anwar wrote: > From: Roger Quadros <rogerq@ti.com> > > This is the Ethernet driver for TI AM654 Silicon rev. 2 > with the ICSSG PRU Sub-system running dual-EMAC firmware. > > The Programmable Real-time Unit and Industrial Communication Subsystem > Gigabit (PRU_ICSSG) is a low-latency microcontroller subsystem in the TI > SoCs. This subsystem is provided for the use cases like implementation of > custom peripheral interfaces, offloading of tasks from the other > processor cores of the SoC, etc. > > Every ICSSG core has two Programmable Real-Time Unit(PRUs), > two auxiliary Real-Time Transfer Unit (RT_PRUs), and > two Transmit Real-Time Transfer Units (TX_PRUs). Each one of these runs > its own firmware. Every ICSSG core has two MII ports connect to these > PRUs and also a MDIO port. > > The cores can run different firmwares to support different protocols and > features like switch-dev, timestamping, etc. > > It uses System DMA to transfer and receive packets and > shared memory register emulation between the firmware and > driver for control and configuration. > > This patch adds support for basic EMAC functionality with 1Gbps > and 100Mbps link speed. 10M and half duplex mode are not supported > currently as they require IEP, the support for which will be added later. > Support for switch-dev, timestamp, etc. will be added later > by subsequent patch series. > > Signed-off-by: Roger Quadros <rogerq@ti.com> > [Vignesh Raghavendra: add 10M full duplex support] > Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com> > [Grygorii Strashko: add support for half duplex operation] > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > Signed-off-by: Puranjay Mohan <p-mohan@ti.com> > Signed-off-by: MD Danish Anwar <danishanwar@ti.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> Hi MD, Thanks for your patch, some review from my side. ... > index 000000000000..27bd92ea8200 > --- /dev/null > +++ b/drivers/net/ethernet/ti/icssg_config.c ... > +static void icssg_config_mii_init(struct prueth_emac *emac) > +{ > + struct prueth *prueth = emac->prueth; > + struct regmap *mii_rt = prueth->mii_rt; > + int slice = prueth_emac_slice(emac); I think you need to check the return value of prueth_emac_slice for errors. Or can that never happen? > + u32 rxcfg_reg, txcfg_reg, pcnt_reg; > + u32 rxcfg, txcfg; For networking code, please arrange local variables in reverse xmas tree order - longest line to shortest. In this case I think that would mean something like: u32 rxcfg, txcfg, rxcfg_reg, txcfg_reg, pcnt_reg; struct prueth *prueth = emac->prueth; int slice = prueth_emac_slice(emac); struct regmap *mii_rt; mii_rt = prueth->mii_rt; You can check this using https://github.com/ecree-solarflare/xmastree > + > + rxcfg_reg = (slice == ICSS_MII0) ? PRUSS_MII_RT_RXCFG0 : > + PRUSS_MII_RT_RXCFG1; > + txcfg_reg = (slice == ICSS_MII0) ? PRUSS_MII_RT_TXCFG0 : > + PRUSS_MII_RT_TXCFG1; > + pcnt_reg = (slice == ICSS_MII0) ? PRUSS_MII_RT_RX_PCNT0 : > + PRUSS_MII_RT_RX_PCNT1; > + > + rxcfg = MII_RXCFG_DEFAULT; > + txcfg = MII_TXCFG_DEFAULT; > + > + if (slice == ICSS_MII1) > + rxcfg |= PRUSS_MII_RT_RXCFG_RX_MUX_SEL; > + > + /* In MII mode TX lines swapped inside ICSSG, so TX_MUX_SEL cfg need > + * to be swapped also comparing to RGMII mode. > + */ > + if (emac->phy_if == PHY_INTERFACE_MODE_MII && slice == ICSS_MII0) > + txcfg |= PRUSS_MII_RT_TXCFG_TX_MUX_SEL; > + else if (emac->phy_if != PHY_INTERFACE_MODE_MII && slice == ICSS_MII1) > + txcfg |= PRUSS_MII_RT_TXCFG_TX_MUX_SEL; > + > + regmap_write(mii_rt, rxcfg_reg, rxcfg); > + regmap_write(mii_rt, txcfg_reg, txcfg); > + regmap_write(mii_rt, pcnt_reg, 0x1); > +} > + > +static void icssg_miig_queues_init(struct prueth *prueth, int slice) > +{ > + struct regmap *miig_rt = prueth->miig_rt; > + void __iomem *smem = prueth->shram.va; > + u8 pd[ICSSG_SPECIAL_PD_SIZE]; > + int queue = 0, i, j; > + u32 *pdword; > + > + /* reset hwqueues */ > + if (slice) > + queue = ICSSG_NUM_TX_QUEUES; > + > + for (i = 0; i < ICSSG_NUM_TX_QUEUES; i++) { > + regmap_write(miig_rt, ICSSG_QUEUE_RESET_OFFSET, queue); > + queue++; > + } > + > + queue = slice ? RECYCLE_Q_SLICE1 : RECYCLE_Q_SLICE0; > + regmap_write(miig_rt, ICSSG_QUEUE_RESET_OFFSET, queue); > + > + for (i = 0; i < ICSSG_NUM_OTHER_QUEUES; i++) { > + regmap_write(miig_rt, ICSSG_QUEUE_RESET_OFFSET, > + hwq_map[slice][i].queue); > + } > + > + /* initialize packet descriptors in SMEM */ > + /* push pakcet descriptors to hwqueues */ > + > + pdword = (u32 *)pd; > + for (j = 0; j < ICSSG_NUM_OTHER_QUEUES; j++) { > + struct map *mp; > + int pd_size, num_pds; > + u32 pdaddr; > + > + mp = &hwq_map[slice][j]; hwq_map is const, but mq is not. clang-16 with W=1 tells me: drivers/net/ethernet/ti/icssg_config.c:176:6: error: assigning to 'struct map *' from 'const struct map *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] mp = &hwq_map[slice][j]; > + if (mp->special) { > + pd_size = ICSSG_SPECIAL_PD_SIZE; > + num_pds = ICSSG_NUM_SPECIAL_PDS; > + } else { > + pd_size = ICSSG_NORMAL_PD_SIZE; > + num_pds = ICSSG_NUM_NORMAL_PDS; > + } > + > + for (i = 0; i < num_pds; i++) { > + memset(pd, 0, pd_size); > + > + pdword[0] &= cpu_to_le32(ICSSG_FLAG_MASK); > + pdword[0] |= cpu_to_le32(mp->flags); > + pdaddr = mp->pd_addr_start + i * pd_size; > + > + memcpy_toio(smem + pdaddr, pd, pd_size); > + queue = mp->queue; > + regmap_write(miig_rt, ICSSG_QUEUE_OFFSET + 4 * queue, > + pdaddr); > + } > + } > +} > + > +void icssg_config_ipg(struct prueth_emac *emac) > +{ > + struct prueth *prueth = emac->prueth; > + int slice = prueth_emac_slice(emac); > + > + switch (emac->speed) { > + case SPEED_1000: > + icssg_mii_update_ipg(prueth->mii_rt, slice, MII_RT_TX_IPG_1G); > + break; > + case SPEED_100: > + icssg_mii_update_ipg(prueth->mii_rt, slice, MII_RT_TX_IPG_100M); > + break; > + default: > + /* Other links speeds not supported */ > + netdev_err(emac->ndev, "Unsupported link speed\n"); > + return; Should this propagate an error to the caller? > + } > +} ... > +static int prueth_emac_buffer_setup(struct prueth_emac *emac) > +{ > + struct icssg_buffer_pool_cfg *bpool_cfg; > + struct prueth *prueth = emac->prueth; > + int slice = prueth_emac_slice(emac); > + struct icssg_rxq_ctx *rxq_ctx; > + u32 addr; > + int i; > + > + /* Layout to have 64KB aligned buffer pool > + * |BPOOL0|BPOOL1|RX_CTX0|RX_CTX1| > + */ > + > + addr = lower_32_bits(prueth->msmcram.pa); > + if (slice) > + addr += PRUETH_NUM_BUF_POOLS * PRUETH_EMAC_BUF_POOL_SIZE; > + > + if (addr % SZ_64K) { > + dev_warn(prueth->dev, "buffer pool needs to be 64KB aligned\n"); > + return -EINVAL; > + } > + > + bpool_cfg = emac->dram.va + BUFFER_POOL_0_ADDR_OFFSET; > + /* workaround for f/w bug. bpool 0 needs to be initilalized */ > + bpool_cfg[0].addr = cpu_to_le32(addr); > + bpool_cfg[0].len = 0; > + > + for (i = PRUETH_EMAC_BUF_POOL_START; > + i < (PRUETH_EMAC_BUF_POOL_START + PRUETH_NUM_BUF_POOLS); nit: unnecessary parentheses > + i++) { > + bpool_cfg[i].addr = cpu_to_le32(addr); > + bpool_cfg[i].len = cpu_to_le32(PRUETH_EMAC_BUF_POOL_SIZE); > + addr += PRUETH_EMAC_BUF_POOL_SIZE; > + } > + > + if (!slice) > + addr += PRUETH_NUM_BUF_POOLS * PRUETH_EMAC_BUF_POOL_SIZE; > + else > + addr += PRUETH_EMAC_RX_CTX_BUF_SIZE * 2; > + > + /* Pre-emptible RX buffer queue */ > + rxq_ctx = emac->dram.va + HOST_RX_Q_PRE_CONTEXT_OFFSET; > + for (i = 0; i < 3; i++) > + rxq_ctx->start[i] = cpu_to_le32(addr); > + > + addr += PRUETH_EMAC_RX_CTX_BUF_SIZE; > + rxq_ctx->end = cpu_to_le32(addr); > + > + /* Express RX buffer queue */ > + rxq_ctx = emac->dram.va + HOST_RX_Q_EXP_CONTEXT_OFFSET; > + for (i = 0; i < 3; i++) > + rxq_ctx->start[i] = cpu_to_le32(addr); > + > + addr += PRUETH_EMAC_RX_CTX_BUF_SIZE; > + rxq_ctx->end = cpu_to_le32(addr); > + > + return 0; > +} ... > +void icssg_config_set_speed(struct prueth_emac *emac) > +{ > + u8 fw_speed; > + > + switch (emac->speed) { > + case SPEED_1000: > + fw_speed = FW_LINK_SPEED_1G; > + break; > + case SPEED_100: > + fw_speed = FW_LINK_SPEED_100M; > + break; > + default: > + /* Other links speeds not supported */ > + netdev_err(emac->ndev, "Unsupported link speed\n"); > + return; Should this propagate an error to the caller? > + } > + > + writeb(fw_speed, emac->dram.va + PORT_LINK_SPEED_OFFSET); > +} ... > diff --git a/drivers/net/ethernet/ti/icssg_prueth.c b/drivers/net/ethernet/ti/icssg_prueth.c ... > +static int prueth_init_rx_chns(struct prueth_emac *emac, > + struct prueth_rx_chn *rx_chn, > + char *name, u32 max_rflows, > + u32 max_desc_num) > +{ > + struct net_device *ndev = emac->ndev; > + struct device *dev = emac->prueth->dev; > + struct k3_udma_glue_rx_channel_cfg rx_cfg; > + u32 fdqring_id; > + u32 hdesc_size; > + int i, ret = 0, slice; > + > + slice = prueth_emac_slice(emac); > + if (slice < 0) > + return slice; > + > + /* To differentiate channels for SLICE0 vs SLICE1 */ > + snprintf(rx_chn->name, sizeof(rx_chn->name), "%s%d", name, slice); > + > + hdesc_size = cppi5_hdesc_calc_size(true, PRUETH_NAV_PS_DATA_SIZE, > + PRUETH_NAV_SW_DATA_SIZE); > + memset(&rx_cfg, 0, sizeof(rx_cfg)); > + rx_cfg.swdata_size = PRUETH_NAV_SW_DATA_SIZE; > + rx_cfg.flow_id_num = max_rflows; > + rx_cfg.flow_id_base = -1; /* udmax will auto select flow id base */ > + > + /* init all flows */ > + rx_chn->dev = dev; > + rx_chn->descs_num = max_desc_num; > + > + rx_chn->rx_chn = k3_udma_glue_request_rx_chn(dev, rx_chn->name, > + &rx_cfg); > + if (IS_ERR(rx_chn->rx_chn)) { > + ret = PTR_ERR(rx_chn->rx_chn); > + rx_chn->rx_chn = NULL; > + netdev_err(ndev, "Failed to request rx dma ch: %d\n", ret); > + goto fail; > + } > + > + rx_chn->dma_dev = k3_udma_glue_rx_get_dma_device(rx_chn->rx_chn); > + rx_chn->desc_pool = k3_cppi_desc_pool_create_name(rx_chn->dma_dev, > + rx_chn->descs_num, > + hdesc_size, > + rx_chn->name); > + if (IS_ERR(rx_chn->desc_pool)) { > + ret = PTR_ERR(rx_chn->desc_pool); > + rx_chn->desc_pool = NULL; > + netdev_err(ndev, "Failed to create rx pool: %d\n", ret); > + goto fail; > + } > + > + emac->rx_flow_id_base = k3_udma_glue_rx_get_flow_id_base(rx_chn->rx_chn); > + netdev_dbg(ndev, "flow id base = %d\n", emac->rx_flow_id_base); > + > + fdqring_id = K3_RINGACC_RING_ID_ANY; > + for (i = 0; i < rx_cfg.flow_id_num; i++) { > + struct k3_ring_cfg rxring_cfg = { > + .elm_size = K3_RINGACC_RING_ELSIZE_8, > + .mode = K3_RINGACC_RING_MODE_RING, > + .flags = 0, > + }; > + struct k3_ring_cfg fdqring_cfg = { > + .elm_size = K3_RINGACC_RING_ELSIZE_8, > + .flags = K3_RINGACC_RING_SHARED, > + }; > + struct k3_udma_glue_rx_flow_cfg rx_flow_cfg = { > + .rx_cfg = rxring_cfg, > + .rxfdq_cfg = fdqring_cfg, > + .ring_rxq_id = K3_RINGACC_RING_ID_ANY, > + .src_tag_lo_sel = > + K3_UDMA_GLUE_SRC_TAG_LO_USE_REMOTE_SRC_TAG, > + }; > + > + rx_flow_cfg.ring_rxfdq0_id = fdqring_id; > + rx_flow_cfg.rx_cfg.size = max_desc_num; > + rx_flow_cfg.rxfdq_cfg.size = max_desc_num; > + rx_flow_cfg.rxfdq_cfg.mode = emac->prueth->pdata.fdqring_mode; > + > + ret = k3_udma_glue_rx_flow_init(rx_chn->rx_chn, > + i, &rx_flow_cfg); > + if (ret) { > + netdev_err(ndev, "Failed to init rx flow%d %d\n", > + i, ret); > + goto fail; > + } > + if (!i) > + fdqring_id = k3_udma_glue_rx_flow_get_fdq_id(rx_chn->rx_chn, > + i); > + rx_chn->irq[i] = k3_udma_glue_rx_get_irq(rx_chn->rx_chn, i); > + if (rx_chn->irq[i] <= 0) { I think ret needs to be set to an error value here here. > + netdev_err(ndev, "Failed to get rx dma irq"); > + goto fail; > + } > + } > + > + return 0; > + > +fail: > + prueth_cleanup_rx_chns(emac, rx_chn, max_rflows); > + return ret; > +} ... > +static void prueth_emac_stop(struct prueth_emac *emac) > +{ > + struct prueth *prueth = emac->prueth; > + int slice; > + > + switch (emac->port_id) { > + case PRUETH_PORT_MII0: > + slice = ICSS_SLICE0; > + break; > + case PRUETH_PORT_MII1: > + slice = ICSS_SLICE1; > + break; > + default: > + netdev_err(emac->ndev, "invalid port\n"); > + return; > + } > + > + emac->fw_running = 0; fw_running won't be cleared if port_id is unknon. Is that ok? Also, it's not obvious to me that fw_running used for anything. > + rproc_shutdown(prueth->txpru[slice]); > + rproc_shutdown(prueth->rtu[slice]); > + rproc_shutdown(prueth->pru[slice]); > +} ... > +static int prueth_netdev_init(struct prueth *prueth, > + struct device_node *eth_node) > +{ > + int ret, num_tx_chn = PRUETH_MAX_TX_QUEUES; > + struct prueth_emac *emac; > + struct net_device *ndev; > + enum prueth_port port; > + enum prueth_mac mac; > + > + port = prueth_node_port(eth_node); > + if (port < 0) > + return -EINVAL; > + > + mac = prueth_node_mac(eth_node); > + if (mac < 0) > + return -EINVAL; > + > + ndev = alloc_etherdev_mq(sizeof(*emac), num_tx_chn); > + if (!ndev) > + return -ENOMEM; > + > + emac = netdev_priv(ndev); > + prueth->emac[mac] = emac; > + emac->prueth = prueth; > + emac->ndev = ndev; > + emac->port_id = port; > + emac->cmd_wq = create_singlethread_workqueue("icssg_cmd_wq"); > + if (!emac->cmd_wq) { > + ret = -ENOMEM; > + goto free_ndev; > + } > + INIT_WORK(&emac->rx_mode_work, emac_ndo_set_rx_mode_work); > + > + ret = pruss_request_mem_region(prueth->pruss, > + port == PRUETH_PORT_MII0 ? > + PRUSS_MEM_DRAM0 : PRUSS_MEM_DRAM1, > + &emac->dram); > + if (ret) { > + dev_err(prueth->dev, "unable to get DRAM: %d\n", ret); > + ret = -ENOMEM; > + goto free_wq; > + } > + > + emac->tx_ch_num = 1; > + > + SET_NETDEV_DEV(ndev, prueth->dev); > + spin_lock_init(&emac->lock); > + mutex_init(&emac->cmd_lock); > + > + emac->phy_node = of_parse_phandle(eth_node, "phy-handle", 0); > + if (!emac->phy_node && !of_phy_is_fixed_link(eth_node)) { > + dev_err(prueth->dev, "couldn't find phy-handle\n"); > + ret = -ENODEV; > + goto free; > + } else if (of_phy_is_fixed_link(eth_node)) { > + ret = of_phy_register_fixed_link(eth_node); > + if (ret) { > + ret = dev_err_probe(prueth->dev, ret, > + "failed to register fixed-link phy\n"); > + goto free; > + } > + > + emac->phy_node = eth_node; > + } > + > + ret = of_get_phy_mode(eth_node, &emac->phy_if); > + if (ret) { > + dev_err(prueth->dev, "could not get phy-mode property\n"); > + goto free; > + } > + > + if (emac->phy_if != PHY_INTERFACE_MODE_MII && > + !phy_interface_mode_is_rgmii(emac->phy_if)) { I think ret needs to be set to an error value here. > + dev_err(prueth->dev, "PHY mode unsupported %s\n", phy_modes(emac->phy_if)); > + goto free; > + } > + > + /* AM65 SR2.0 has TX Internal delay always enabled by hardware > + * and it is not possible to disable TX Internal delay. The below > + * switch case block describes how we handle different phy modes > + * based on hardware restriction. > + */ > + switch (emac->phy_if) { > + case PHY_INTERFACE_MODE_RGMII_ID: > + emac->phy_if = PHY_INTERFACE_MODE_RGMII_RXID; > + break; > + case PHY_INTERFACE_MODE_RGMII_TXID: > + emac->phy_if = PHY_INTERFACE_MODE_RGMII; > + break; > + case PHY_INTERFACE_MODE_RGMII: > + case PHY_INTERFACE_MODE_RGMII_RXID: > + dev_err(prueth->dev, "RGMII mode without TX delay is not supported"); > + return -EINVAL; > + default: Shoud this be an error condition? > + break; > + } > + > + /* get mac address from DT and set private and netdev addr */ > + ret = of_get_ethdev_address(eth_node, ndev); > + if (!is_valid_ether_addr(ndev->dev_addr)) { > + eth_hw_addr_random(ndev); > + dev_warn(prueth->dev, "port %d: using random MAC addr: %pM\n", > + port, ndev->dev_addr); > + } > + ether_addr_copy(emac->mac_addr, ndev->dev_addr); > + > + ndev->netdev_ops = &emac_netdev_ops; > + ndev->ethtool_ops = &icssg_ethtool_ops; > + ndev->hw_features = NETIF_F_SG; > + ndev->features = ndev->hw_features; > + > + netif_napi_add(ndev, &emac->napi_rx, > + emac_napi_rx_poll); > + > + return 0; > + > +free: > + pruss_release_mem_region(prueth->pruss, &emac->dram); > +free_wq: > + destroy_workqueue(emac->cmd_wq); > +free_ndev: > + free_netdev(ndev); > + prueth->emac[mac] = NULL; > + > + return ret; > +} ... > +static int prueth_probe(struct platform_device *pdev) > +{ > + struct prueth *prueth; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct device_node *eth_ports_node; > + struct device_node *eth_node; > + struct device_node *eth0_node, *eth1_node; > + const struct of_device_id *match; > + struct pruss *pruss; > + int i, ret; > + u32 msmc_ram_size; > + struct genpool_data_align gp_data = { > + .align = SZ_64K, > + }; > + > + match = of_match_device(prueth_dt_match, dev); > + if (!match) > + return -ENODEV; > + > + prueth = devm_kzalloc(dev, sizeof(*prueth), GFP_KERNEL); > + if (!prueth) > + return -ENOMEM; > + > + dev_set_drvdata(dev, prueth); > + prueth->pdev = pdev; > + prueth->pdata = *(const struct prueth_pdata *)match->data; > + > + prueth->dev = dev; > + eth_ports_node = of_get_child_by_name(np, "ethernet-ports"); > + if (!eth_ports_node) > + return -ENOENT; > + > + for_each_child_of_node(eth_ports_node, eth_node) { > + u32 reg; > + > + if (strcmp(eth_node->name, "port")) > + continue; > + ret = of_property_read_u32(eth_node, "reg", ®); > + if (ret < 0) { > + dev_err(dev, "%pOF error reading port_id %d\n", > + eth_node, ret); > + } > + > + of_node_get(eth_node); > + > + if (reg == 0) > + eth0_node = eth_node; > + else if (reg == 1) > + eth1_node = eth_node; > + else > + dev_err(dev, "port reg should be 0 or 1\n"); > + } > + > + of_node_put(eth_ports_node); > + > + /* At least one node must be present and available else we fail */ > + if (!eth0_node && !eth1_node) { Are eth0_node and eth1_node always initialised in the loop above? > + dev_err(dev, "neither port0 nor port1 node available\n"); > + return -ENODEV; > + } > + > + if (eth0_node == eth1_node) { > + dev_err(dev, "port0 and port1 can't have same reg\n"); > + of_node_put(eth0_node); > + return -ENODEV; > + } ... > +MODULE_AUTHOR("Roger Quadros <rogerq@ti.com>"); > +MODULE_AUTHOR("Puranjay Mohan <p-mohan@ti.com>"); > +MODULE_AUTHOR("Md Danish Anwar <danishanwar@ti.com>"); > +MODULE_DESCRIPTION("PRUSS ICSSG Ethernet Driver"); > +MODULE_LICENSE("GPL"); SPDK says GPL-2.0, so perhaps this should be "GPL v2" ? > diff --git a/drivers/net/ethernet/ti/icssg_prueth.h b/drivers/net/ethernet/ti/icssg_prueth.h ... > +/** > + * struct prueth - PRUeth platform data nit: s/prueth/prueth_pdata/ > + * @fdqring_mode: Free desc queue mode > + * @quirk_10m_link_issue: 10M link detect errata > + */ > +struct prueth_pdata { > + enum k3_ring_mode fdqring_mode; > + u32 quirk_10m_link_issue:1; > +}; > + > +/** > + * struct prueth - PRUeth structure > + * @dev: device > + * @pruss: pruss handle > + * @pru: rproc instances of PRUs > + * @rtu: rproc instances of RTUs > + * @rtu: rproc instances of TX_PRUs nit: txpru is missing here. > + * @shram: PRUSS shared RAM region > + * @sram_pool: MSMC RAM pool for buffers > + * @msmcram: MSMC RAM region > + * @eth_node: DT node for the port > + * @emac: private EMAC data structure > + * @registered_netdevs: list of registered netdevs > + * @fw_data: firmware names to be used with PRU remoteprocs > + * @config: firmware load time configuration per slice > + * @miig_rt: regmap to mii_g_rt block nit: mii_rt is missing here. > + * @pa_stats: regmap to pa_stats block > + * @pru_id: ID for each of the PRUs > + * @pdev: pointer to ICSSG platform device > + * @pdata: pointer to platform data for ICSSG driver > + * @icssg_hwcmdseq: seq counter or HWQ messages > + * @emacs_initialized: num of EMACs/ext ports that are up/running > + */ > +struct prueth { > + struct device *dev; > + struct pruss *pruss; > + struct rproc *pru[PRUSS_NUM_PRUS]; > + struct rproc *rtu[PRUSS_NUM_PRUS]; > + struct rproc *txpru[PRUSS_NUM_PRUS]; > + struct pruss_mem_region shram; > + struct gen_pool *sram_pool; > + struct pruss_mem_region msmcram; > + > + struct device_node *eth_node[PRUETH_NUM_MACS]; > + struct prueth_emac *emac[PRUETH_NUM_MACS]; > + struct net_device *registered_netdevs[PRUETH_NUM_MACS]; > + const struct prueth_private_data *fw_data; > + struct regmap *miig_rt; > + struct regmap *mii_rt; > + struct regmap *pa_stats; > + > + enum pruss_pru_id pru_id[PRUSS_NUM_PRUS]; > + struct platform_device *pdev; > + struct prueth_pdata pdata; > + u8 icssg_hwcmdseq; > + > + int emacs_initialized; > +}; ...
Hi Simon, Thanks for the comments. On 27/04/23 00:39, Simon Horman wrote: > On Mon, Apr 24, 2023 at 11:02:33AM +0530, MD Danish Anwar wrote: >> From: Roger Quadros <rogerq@ti.com> >> >> This is the Ethernet driver for TI AM654 Silicon rev. 2 >> with the ICSSG PRU Sub-system running dual-EMAC firmware. >> >> The Programmable Real-time Unit and Industrial Communication Subsystem >> Gigabit (PRU_ICSSG) is a low-latency microcontroller subsystem in the TI >> SoCs. This subsystem is provided for the use cases like implementation of >> custom peripheral interfaces, offloading of tasks from the other >> processor cores of the SoC, etc. >> >> Every ICSSG core has two Programmable Real-Time Unit(PRUs), >> two auxiliary Real-Time Transfer Unit (RT_PRUs), and >> two Transmit Real-Time Transfer Units (TX_PRUs). Each one of these runs >> its own firmware. Every ICSSG core has two MII ports connect to these >> PRUs and also a MDIO port. >> >> The cores can run different firmwares to support different protocols and >> features like switch-dev, timestamping, etc. >> >> It uses System DMA to transfer and receive packets and >> shared memory register emulation between the firmware and >> driver for control and configuration. >> >> This patch adds support for basic EMAC functionality with 1Gbps >> and 100Mbps link speed. 10M and half duplex mode are not supported >> currently as they require IEP, the support for which will be added later. >> Support for switch-dev, timestamp, etc. will be added later >> by subsequent patch series. >> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> [Vignesh Raghavendra: add 10M full duplex support] >> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com> >> [Grygorii Strashko: add support for half duplex operation] >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> Signed-off-by: Puranjay Mohan <p-mohan@ti.com> >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >> Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Hi MD, > > Thanks for your patch, some review from my side. > > ... > >> index 000000000000..27bd92ea8200 >> --- /dev/null >> +++ b/drivers/net/ethernet/ti/icssg_config.c > > ... > >> +static void icssg_config_mii_init(struct prueth_emac *emac) >> +{ >> + struct prueth *prueth = emac->prueth; >> + struct regmap *mii_rt = prueth->mii_rt; >> + int slice = prueth_emac_slice(emac); > > I think you need to check the return value of prueth_emac_slice for errors. > Or can that never happen? > The value of slice can not be invalid here. icssg_config_mii_init() is called via ndo open which is called after prueth_probe(). The slices / ports are initialized in prueth_probe() --> prueth_netdev_init(). If the slices / ports are not as expected, prueth_netdev_init() will return -EINVAL as a result prueth_probe() will also return with error, probe will not be complete, ndo_open will not be called thus the call flow will never reach here. icssg_config_mii_init() API getting called implies that prueth_probe was successfull, the ports / slices are vaild and prueth_emac_slice() will not return errors. So no need to check for errors here I think. >> + u32 rxcfg_reg, txcfg_reg, pcnt_reg; >> + u32 rxcfg, txcfg; > > For networking code, please arrange local variables in reverse xmas tree > order - longest line to shortest. In this case I think that would mean > something like: > > u32 rxcfg, txcfg, rxcfg_reg, txcfg_reg, pcnt_reg; > struct prueth *prueth = emac->prueth; > int slice = prueth_emac_slice(emac); > struct regmap *mii_rt; > > mii_rt = prueth->mii_rt; > > You can check this using https://github.com/ecree-solarflare/xmastree > Sure, I will try to follow reverse xmas tree order. >> + >> + rxcfg_reg = (slice == ICSS_MII0) ? PRUSS_MII_RT_RXCFG0 : >> + PRUSS_MII_RT_RXCFG1; >> + txcfg_reg = (slice == ICSS_MII0) ? PRUSS_MII_RT_TXCFG0 : >> + PRUSS_MII_RT_TXCFG1; >> + pcnt_reg = (slice == ICSS_MII0) ? PRUSS_MII_RT_RX_PCNT0 : >> + PRUSS_MII_RT_RX_PCNT1; >> + >> + rxcfg = MII_RXCFG_DEFAULT; >> + txcfg = MII_TXCFG_DEFAULT; >> + >> + if (slice == ICSS_MII1) >> + rxcfg |= PRUSS_MII_RT_RXCFG_RX_MUX_SEL; >> + >> + /* In MII mode TX lines swapped inside ICSSG, so TX_MUX_SEL cfg need >> + * to be swapped also comparing to RGMII mode. >> + */ >> + if (emac->phy_if == PHY_INTERFACE_MODE_MII && slice == ICSS_MII0) >> + txcfg |= PRUSS_MII_RT_TXCFG_TX_MUX_SEL; >> + else if (emac->phy_if != PHY_INTERFACE_MODE_MII && slice == ICSS_MII1) >> + txcfg |= PRUSS_MII_RT_TXCFG_TX_MUX_SEL; >> + >> + regmap_write(mii_rt, rxcfg_reg, rxcfg); >> + regmap_write(mii_rt, txcfg_reg, txcfg); >> + regmap_write(mii_rt, pcnt_reg, 0x1); >> +} >> + >> +static void icssg_miig_queues_init(struct prueth *prueth, int slice) >> +{ >> + struct regmap *miig_rt = prueth->miig_rt; >> + void __iomem *smem = prueth->shram.va; >> + u8 pd[ICSSG_SPECIAL_PD_SIZE]; >> + int queue = 0, i, j; >> + u32 *pdword; >> + >> + /* reset hwqueues */ >> + if (slice) >> + queue = ICSSG_NUM_TX_QUEUES; >> + >> + for (i = 0; i < ICSSG_NUM_TX_QUEUES; i++) { >> + regmap_write(miig_rt, ICSSG_QUEUE_RESET_OFFSET, queue); >> + queue++; >> + } >> + >> + queue = slice ? RECYCLE_Q_SLICE1 : RECYCLE_Q_SLICE0; >> + regmap_write(miig_rt, ICSSG_QUEUE_RESET_OFFSET, queue); >> + >> + for (i = 0; i < ICSSG_NUM_OTHER_QUEUES; i++) { >> + regmap_write(miig_rt, ICSSG_QUEUE_RESET_OFFSET, >> + hwq_map[slice][i].queue); >> + } >> + >> + /* initialize packet descriptors in SMEM */ >> + /* push pakcet descriptors to hwqueues */ >> + >> + pdword = (u32 *)pd; >> + for (j = 0; j < ICSSG_NUM_OTHER_QUEUES; j++) { >> + struct map *mp; >> + int pd_size, num_pds; >> + u32 pdaddr; >> + >> + mp = &hwq_map[slice][j]; > > hwq_map is const, but mq is not. > > clang-16 with W=1 tells me: > > drivers/net/ethernet/ti/icssg_config.c:176:6: error: assigning to 'struct map *' from 'const struct map *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] > mp = &hwq_map[slice][j]; > I will make 'mp' as const as well. >> + if (mp->special) { >> + pd_size = ICSSG_SPECIAL_PD_SIZE; >> + num_pds = ICSSG_NUM_SPECIAL_PDS; >> + } else { >> + pd_size = ICSSG_NORMAL_PD_SIZE; >> + num_pds = ICSSG_NUM_NORMAL_PDS; >> + } >> + >> + for (i = 0; i < num_pds; i++) { >> + memset(pd, 0, pd_size); >> + >> + pdword[0] &= cpu_to_le32(ICSSG_FLAG_MASK); >> + pdword[0] |= cpu_to_le32(mp->flags); >> + pdaddr = mp->pd_addr_start + i * pd_size; >> + >> + memcpy_toio(smem + pdaddr, pd, pd_size); >> + queue = mp->queue; >> + regmap_write(miig_rt, ICSSG_QUEUE_OFFSET + 4 * queue, >> + pdaddr); >> + } >> + } >> +} >> + >> +void icssg_config_ipg(struct prueth_emac *emac) >> +{ >> + struct prueth *prueth = emac->prueth; >> + int slice = prueth_emac_slice(emac); >> + >> + switch (emac->speed) { >> + case SPEED_1000: >> + icssg_mii_update_ipg(prueth->mii_rt, slice, MII_RT_TX_IPG_1G); >> + break; >> + case SPEED_100: >> + icssg_mii_update_ipg(prueth->mii_rt, slice, MII_RT_TX_IPG_100M); >> + break; >> + default: >> + /* Other links speeds not supported */ >> + netdev_err(emac->ndev, "Unsupported link speed\n"); >> + return; > > Should this propagate an error to the caller? > I don't think this should propagate an error. >> + } >> +} > > ... > >> +static int prueth_emac_buffer_setup(struct prueth_emac *emac) >> +{ >> + struct icssg_buffer_pool_cfg *bpool_cfg; >> + struct prueth *prueth = emac->prueth; >> + int slice = prueth_emac_slice(emac); >> + struct icssg_rxq_ctx *rxq_ctx; >> + u32 addr; >> + int i; >> + >> + /* Layout to have 64KB aligned buffer pool >> + * |BPOOL0|BPOOL1|RX_CTX0|RX_CTX1| >> + */ >> + >> + addr = lower_32_bits(prueth->msmcram.pa); >> + if (slice) >> + addr += PRUETH_NUM_BUF_POOLS * PRUETH_EMAC_BUF_POOL_SIZE; >> + >> + if (addr % SZ_64K) { >> + dev_warn(prueth->dev, "buffer pool needs to be 64KB aligned\n"); >> + return -EINVAL; >> + } >> + >> + bpool_cfg = emac->dram.va + BUFFER_POOL_0_ADDR_OFFSET; >> + /* workaround for f/w bug. bpool 0 needs to be initilalized */ >> + bpool_cfg[0].addr = cpu_to_le32(addr); >> + bpool_cfg[0].len = 0; >> + >> + for (i = PRUETH_EMAC_BUF_POOL_START; >> + i < (PRUETH_EMAC_BUF_POOL_START + PRUETH_NUM_BUF_POOLS); > > nit: unnecessary parentheses > >> + i++) { >> + bpool_cfg[i].addr = cpu_to_le32(addr); >> + bpool_cfg[i].len = cpu_to_le32(PRUETH_EMAC_BUF_POOL_SIZE); >> + addr += PRUETH_EMAC_BUF_POOL_SIZE; >> + } >> + >> + if (!slice) >> + addr += PRUETH_NUM_BUF_POOLS * PRUETH_EMAC_BUF_POOL_SIZE; >> + else >> + addr += PRUETH_EMAC_RX_CTX_BUF_SIZE * 2; >> + >> + /* Pre-emptible RX buffer queue */ >> + rxq_ctx = emac->dram.va + HOST_RX_Q_PRE_CONTEXT_OFFSET; >> + for (i = 0; i < 3; i++) >> + rxq_ctx->start[i] = cpu_to_le32(addr); >> + >> + addr += PRUETH_EMAC_RX_CTX_BUF_SIZE; >> + rxq_ctx->end = cpu_to_le32(addr); >> + >> + /* Express RX buffer queue */ >> + rxq_ctx = emac->dram.va + HOST_RX_Q_EXP_CONTEXT_OFFSET; >> + for (i = 0; i < 3; i++) >> + rxq_ctx->start[i] = cpu_to_le32(addr); >> + >> + addr += PRUETH_EMAC_RX_CTX_BUF_SIZE; >> + rxq_ctx->end = cpu_to_le32(addr); >> + >> + return 0; >> +} > > ... > >> +void icssg_config_set_speed(struct prueth_emac *emac) >> +{ >> + u8 fw_speed; >> + >> + switch (emac->speed) { >> + case SPEED_1000: >> + fw_speed = FW_LINK_SPEED_1G; >> + break; >> + case SPEED_100: >> + fw_speed = FW_LINK_SPEED_100M; >> + break; >> + default: >> + /* Other links speeds not supported */ >> + netdev_err(emac->ndev, "Unsupported link speed\n"); >> + return; > > Should this propagate an error to the caller? > I don't think this should propagate an error. >> + } >> + >> + writeb(fw_speed, emac->dram.va + PORT_LINK_SPEED_OFFSET); >> +} > > ... > >> diff --git a/drivers/net/ethernet/ti/icssg_prueth.c b/drivers/net/ethernet/ti/icssg_prueth.c > > ... > >> +static int prueth_init_rx_chns(struct prueth_emac *emac, >> + struct prueth_rx_chn *rx_chn, >> + char *name, u32 max_rflows, >> + u32 max_desc_num) >> +{ >> + struct net_device *ndev = emac->ndev; >> + struct device *dev = emac->prueth->dev; >> + struct k3_udma_glue_rx_channel_cfg rx_cfg; >> + u32 fdqring_id; >> + u32 hdesc_size; >> + int i, ret = 0, slice; >> + >> + slice = prueth_emac_slice(emac); >> + if (slice < 0) >> + return slice; >> + >> + /* To differentiate channels for SLICE0 vs SLICE1 */ >> + snprintf(rx_chn->name, sizeof(rx_chn->name), "%s%d", name, slice); >> + >> + hdesc_size = cppi5_hdesc_calc_size(true, PRUETH_NAV_PS_DATA_SIZE, >> + PRUETH_NAV_SW_DATA_SIZE); >> + memset(&rx_cfg, 0, sizeof(rx_cfg)); >> + rx_cfg.swdata_size = PRUETH_NAV_SW_DATA_SIZE; >> + rx_cfg.flow_id_num = max_rflows; >> + rx_cfg.flow_id_base = -1; /* udmax will auto select flow id base */ >> + >> + /* init all flows */ >> + rx_chn->dev = dev; >> + rx_chn->descs_num = max_desc_num; >> + >> + rx_chn->rx_chn = k3_udma_glue_request_rx_chn(dev, rx_chn->name, >> + &rx_cfg); >> + if (IS_ERR(rx_chn->rx_chn)) { >> + ret = PTR_ERR(rx_chn->rx_chn); >> + rx_chn->rx_chn = NULL; >> + netdev_err(ndev, "Failed to request rx dma ch: %d\n", ret); >> + goto fail; >> + } >> + >> + rx_chn->dma_dev = k3_udma_glue_rx_get_dma_device(rx_chn->rx_chn); >> + rx_chn->desc_pool = k3_cppi_desc_pool_create_name(rx_chn->dma_dev, >> + rx_chn->descs_num, >> + hdesc_size, >> + rx_chn->name); >> + if (IS_ERR(rx_chn->desc_pool)) { >> + ret = PTR_ERR(rx_chn->desc_pool); >> + rx_chn->desc_pool = NULL; >> + netdev_err(ndev, "Failed to create rx pool: %d\n", ret); >> + goto fail; >> + } >> + >> + emac->rx_flow_id_base = k3_udma_glue_rx_get_flow_id_base(rx_chn->rx_chn); >> + netdev_dbg(ndev, "flow id base = %d\n", emac->rx_flow_id_base); >> + >> + fdqring_id = K3_RINGACC_RING_ID_ANY; >> + for (i = 0; i < rx_cfg.flow_id_num; i++) { >> + struct k3_ring_cfg rxring_cfg = { >> + .elm_size = K3_RINGACC_RING_ELSIZE_8, >> + .mode = K3_RINGACC_RING_MODE_RING, >> + .flags = 0, >> + }; >> + struct k3_ring_cfg fdqring_cfg = { >> + .elm_size = K3_RINGACC_RING_ELSIZE_8, >> + .flags = K3_RINGACC_RING_SHARED, >> + }; >> + struct k3_udma_glue_rx_flow_cfg rx_flow_cfg = { >> + .rx_cfg = rxring_cfg, >> + .rxfdq_cfg = fdqring_cfg, >> + .ring_rxq_id = K3_RINGACC_RING_ID_ANY, >> + .src_tag_lo_sel = >> + K3_UDMA_GLUE_SRC_TAG_LO_USE_REMOTE_SRC_TAG, >> + }; >> + >> + rx_flow_cfg.ring_rxfdq0_id = fdqring_id; >> + rx_flow_cfg.rx_cfg.size = max_desc_num; >> + rx_flow_cfg.rxfdq_cfg.size = max_desc_num; >> + rx_flow_cfg.rxfdq_cfg.mode = emac->prueth->pdata.fdqring_mode; >> + >> + ret = k3_udma_glue_rx_flow_init(rx_chn->rx_chn, >> + i, &rx_flow_cfg); >> + if (ret) { >> + netdev_err(ndev, "Failed to init rx flow%d %d\n", >> + i, ret); >> + goto fail; >> + } >> + if (!i) >> + fdqring_id = k3_udma_glue_rx_flow_get_fdq_id(rx_chn->rx_chn, >> + i); >> + rx_chn->irq[i] = k3_udma_glue_rx_get_irq(rx_chn->rx_chn, i); >> + if (rx_chn->irq[i] <= 0) { > > I think ret needs to be set to an error value here here. > Sure, I will set ret here in this if block. The if block will be like this. if (rx_chn->irq[i] <= 0) { ret = rx_chn->irq[i]; netdev_err(ndev, "Failed to get rx dma irq"); goto fail; } >> + netdev_err(ndev, "Failed to get rx dma irq"); >> + goto fail; >> + } >> + } >> + >> + return 0; >> + >> +fail: >> + prueth_cleanup_rx_chns(emac, rx_chn, max_rflows); >> + return ret; >> +} > > ... > >> +static void prueth_emac_stop(struct prueth_emac *emac) >> +{ >> + struct prueth *prueth = emac->prueth; >> + int slice; >> + >> + switch (emac->port_id) { >> + case PRUETH_PORT_MII0: >> + slice = ICSS_SLICE0; >> + break; >> + case PRUETH_PORT_MII1: >> + slice = ICSS_SLICE1; >> + break; >> + default: >> + netdev_err(emac->ndev, "invalid port\n"); >> + return; >> + } >> + >> + emac->fw_running = 0; > > fw_running won't be cleared if port_id is unknon. Is that ok? If port_id is unknown or invalid. FW running will not be set at all. So no need to clear it, if port_id is invalid. > Also, it's not obvious to me that fw_running used for anything. fw_running will be used in ICSS IEP driver ( which I will introduce upstream once this series gets megred). > >> + rproc_shutdown(prueth->txpru[slice]); >> + rproc_shutdown(prueth->rtu[slice]); >> + rproc_shutdown(prueth->pru[slice]); >> +} > > ... > >> +static int prueth_netdev_init(struct prueth *prueth, >> + struct device_node *eth_node) >> +{ >> + int ret, num_tx_chn = PRUETH_MAX_TX_QUEUES; >> + struct prueth_emac *emac; >> + struct net_device *ndev; >> + enum prueth_port port; >> + enum prueth_mac mac; >> + >> + port = prueth_node_port(eth_node); >> + if (port < 0) >> + return -EINVAL; >> + >> + mac = prueth_node_mac(eth_node); >> + if (mac < 0) >> + return -EINVAL; >> + >> + ndev = alloc_etherdev_mq(sizeof(*emac), num_tx_chn); >> + if (!ndev) >> + return -ENOMEM; >> + >> + emac = netdev_priv(ndev); >> + prueth->emac[mac] = emac; >> + emac->prueth = prueth; >> + emac->ndev = ndev; >> + emac->port_id = port; >> + emac->cmd_wq = create_singlethread_workqueue("icssg_cmd_wq"); >> + if (!emac->cmd_wq) { >> + ret = -ENOMEM; >> + goto free_ndev; >> + } >> + INIT_WORK(&emac->rx_mode_work, emac_ndo_set_rx_mode_work); >> + >> + ret = pruss_request_mem_region(prueth->pruss, >> + port == PRUETH_PORT_MII0 ? >> + PRUSS_MEM_DRAM0 : PRUSS_MEM_DRAM1, >> + &emac->dram); >> + if (ret) { >> + dev_err(prueth->dev, "unable to get DRAM: %d\n", ret); >> + ret = -ENOMEM; >> + goto free_wq; >> + } >> + >> + emac->tx_ch_num = 1; >> + >> + SET_NETDEV_DEV(ndev, prueth->dev); >> + spin_lock_init(&emac->lock); >> + mutex_init(&emac->cmd_lock); >> + >> + emac->phy_node = of_parse_phandle(eth_node, "phy-handle", 0); >> + if (!emac->phy_node && !of_phy_is_fixed_link(eth_node)) { >> + dev_err(prueth->dev, "couldn't find phy-handle\n"); >> + ret = -ENODEV; >> + goto free; >> + } else if (of_phy_is_fixed_link(eth_node)) { >> + ret = of_phy_register_fixed_link(eth_node); >> + if (ret) { >> + ret = dev_err_probe(prueth->dev, ret, >> + "failed to register fixed-link phy\n"); >> + goto free; >> + } >> + >> + emac->phy_node = eth_node; >> + } >> + >> + ret = of_get_phy_mode(eth_node, &emac->phy_if); >> + if (ret) { >> + dev_err(prueth->dev, "could not get phy-mode property\n"); >> + goto free; >> + } >> + >> + if (emac->phy_if != PHY_INTERFACE_MODE_MII && >> + !phy_interface_mode_is_rgmii(emac->phy_if)) { > > I think ret needs to be set to an error value here. > Sure, I will set ret to -EINVAL here. >> + dev_err(prueth->dev, "PHY mode unsupported %s\n", phy_modes(emac->phy_if)); >> + goto free; >> + } >> + >> + /* AM65 SR2.0 has TX Internal delay always enabled by hardware >> + * and it is not possible to disable TX Internal delay. The below >> + * switch case block describes how we handle different phy modes >> + * based on hardware restriction. >> + */ >> + switch (emac->phy_if) { >> + case PHY_INTERFACE_MODE_RGMII_ID: >> + emac->phy_if = PHY_INTERFACE_MODE_RGMII_RXID; >> + break; >> + case PHY_INTERFACE_MODE_RGMII_TXID: >> + emac->phy_if = PHY_INTERFACE_MODE_RGMII; >> + break; >> + case PHY_INTERFACE_MODE_RGMII: >> + case PHY_INTERFACE_MODE_RGMII_RXID: >> + dev_err(prueth->dev, "RGMII mode without TX delay is not supported"); >> + return -EINVAL; >> + default: > > Shoud this be an error condition? > No, I don't think. We only need special handling for different RGMII modes. Default case here means, it's not rgmii mode, i.e. the phy-mode is mii mode which is supported and needs no special handling. The switch case will not do any thing in that scenario and pass the same phy-mode to phy via emac_phy_connect(). >> + break; >> + } >> + >> + /* get mac address from DT and set private and netdev addr */ >> + ret = of_get_ethdev_address(eth_node, ndev); >> + if (!is_valid_ether_addr(ndev->dev_addr)) { >> + eth_hw_addr_random(ndev); >> + dev_warn(prueth->dev, "port %d: using random MAC addr: %pM\n", >> + port, ndev->dev_addr); >> + } >> + ether_addr_copy(emac->mac_addr, ndev->dev_addr); >> + >> + ndev->netdev_ops = &emac_netdev_ops; >> + ndev->ethtool_ops = &icssg_ethtool_ops; >> + ndev->hw_features = NETIF_F_SG; >> + ndev->features = ndev->hw_features; >> + >> + netif_napi_add(ndev, &emac->napi_rx, >> + emac_napi_rx_poll); >> + >> + return 0; >> + >> +free: >> + pruss_release_mem_region(prueth->pruss, &emac->dram); >> +free_wq: >> + destroy_workqueue(emac->cmd_wq); >> +free_ndev: >> + free_netdev(ndev); >> + prueth->emac[mac] = NULL; >> + >> + return ret; >> +} > > ... > >> +static int prueth_probe(struct platform_device *pdev) >> +{ >> + struct prueth *prueth; >> + struct device *dev = &pdev->dev; >> + struct device_node *np = dev->of_node; >> + struct device_node *eth_ports_node; >> + struct device_node *eth_node; >> + struct device_node *eth0_node, *eth1_node; >> + const struct of_device_id *match; >> + struct pruss *pruss; >> + int i, ret; >> + u32 msmc_ram_size; >> + struct genpool_data_align gp_data = { >> + .align = SZ_64K, >> + }; >> + >> + match = of_match_device(prueth_dt_match, dev); >> + if (!match) >> + return -ENODEV; >> + >> + prueth = devm_kzalloc(dev, sizeof(*prueth), GFP_KERNEL); >> + if (!prueth) >> + return -ENOMEM; >> + >> + dev_set_drvdata(dev, prueth); >> + prueth->pdev = pdev; >> + prueth->pdata = *(const struct prueth_pdata *)match->data; >> + >> + prueth->dev = dev; >> + eth_ports_node = of_get_child_by_name(np, "ethernet-ports"); >> + if (!eth_ports_node) >> + return -ENOENT; >> + >> + for_each_child_of_node(eth_ports_node, eth_node) { >> + u32 reg; >> + >> + if (strcmp(eth_node->name, "port")) >> + continue; >> + ret = of_property_read_u32(eth_node, "reg", ®); >> + if (ret < 0) { >> + dev_err(dev, "%pOF error reading port_id %d\n", >> + eth_node, ret); >> + } >> + >> + of_node_get(eth_node); >> + >> + if (reg == 0) >> + eth0_node = eth_node; >> + else if (reg == 1) >> + eth1_node = eth_node; >> + else >> + dev_err(dev, "port reg should be 0 or 1\n"); >> + } >> + >> + of_node_put(eth_ports_node); >> + >> + /* At least one node must be present and available else we fail */ >> + if (!eth0_node && !eth1_node) { > > Are eth0_node and eth1_node always initialised in the loop above? > Yes both eth0_node and eth1_node are initialized in the above loop always. If both of them are NULL or both of them are same. Probe will return with -ENODEV. If atleast one of them is valid we will continue probe and ICSSG driver will work in Single / Dual EMAC mode. >> + dev_err(dev, "neither port0 nor port1 node available\n"); >> + return -ENODEV; >> + } >> + >> + if (eth0_node == eth1_node) { >> + dev_err(dev, "port0 and port1 can't have same reg\n"); >> + of_node_put(eth0_node); >> + return -ENODEV; >> + } > > ... > >> +MODULE_AUTHOR("Roger Quadros <rogerq@ti.com>"); >> +MODULE_AUTHOR("Puranjay Mohan <p-mohan@ti.com>"); >> +MODULE_AUTHOR("Md Danish Anwar <danishanwar@ti.com>"); >> +MODULE_DESCRIPTION("PRUSS ICSSG Ethernet Driver"); >> +MODULE_LICENSE("GPL"); > > SPDK says GPL-2.0, so perhaps this should be "GPL v2" ? > Sure. I will change it. >> diff --git a/drivers/net/ethernet/ti/icssg_prueth.h b/drivers/net/ethernet/ti/icssg_prueth.h > > ... > >> +/** >> + * struct prueth - PRUeth platform data > > nit: s/prueth/prueth_pdata/ > Sure. >> + * @fdqring_mode: Free desc queue mode >> + * @quirk_10m_link_issue: 10M link detect errata >> + */ >> +struct prueth_pdata { >> + enum k3_ring_mode fdqring_mode; >> + u32 quirk_10m_link_issue:1; >> +}; >> + >> +/** >> + * struct prueth - PRUeth structure >> + * @dev: device >> + * @pruss: pruss handle >> + * @pru: rproc instances of PRUs >> + * @rtu: rproc instances of RTUs >> + * @rtu: rproc instances of TX_PRUs > > nit: txpru is missing here. > Yes, it's a typo. I will fix it. >> + * @shram: PRUSS shared RAM region >> + * @sram_pool: MSMC RAM pool for buffers >> + * @msmcram: MSMC RAM region >> + * @eth_node: DT node for the port >> + * @emac: private EMAC data structure >> + * @registered_netdevs: list of registered netdevs >> + * @fw_data: firmware names to be used with PRU remoteprocs >> + * @config: firmware load time configuration per slice >> + * @miig_rt: regmap to mii_g_rt block > > nit: mii_rt is missing here. > I will add it. >> + * @pa_stats: regmap to pa_stats block >> + * @pru_id: ID for each of the PRUs >> + * @pdev: pointer to ICSSG platform device >> + * @pdata: pointer to platform data for ICSSG driver >> + * @icssg_hwcmdseq: seq counter or HWQ messages >> + * @emacs_initialized: num of EMACs/ext ports that are up/running >> + */ >> +struct prueth { >> + struct device *dev; >> + struct pruss *pruss; >> + struct rproc *pru[PRUSS_NUM_PRUS]; >> + struct rproc *rtu[PRUSS_NUM_PRUS]; >> + struct rproc *txpru[PRUSS_NUM_PRUS]; >> + struct pruss_mem_region shram; >> + struct gen_pool *sram_pool; >> + struct pruss_mem_region msmcram; >> + >> + struct device_node *eth_node[PRUETH_NUM_MACS]; >> + struct prueth_emac *emac[PRUETH_NUM_MACS]; >> + struct net_device *registered_netdevs[PRUETH_NUM_MACS]; >> + const struct prueth_private_data *fw_data; >> + struct regmap *miig_rt; >> + struct regmap *mii_rt; >> + struct regmap *pa_stats; >> + >> + enum pruss_pru_id pru_id[PRUSS_NUM_PRUS]; >> + struct platform_device *pdev; >> + struct prueth_pdata pdata; >> + u8 icssg_hwcmdseq; >> + >> + int emacs_initialized; >> +}; > > ... Please let me know if any other change is required. I will adress these changes in next revision.
On Thu, Apr 27, 2023 at 12:42:56PM +0530, Md Danish Anwar wrote: > Hi Simon, > Thanks for the comments. ... > Please let me know if any other change is required. I will adress these changes > in next revision. Hi, Thanks for responding to my review. I have no further requests at this time.
Hi Simon. On 27/04/23 12:42, Md Danish Anwar wrote: > Hi Simon, > Thanks for the comments. > > On 27/04/23 00:39, Simon Horman wrote: >> On Mon, Apr 24, 2023 at 11:02:33AM +0530, MD Danish Anwar wrote: >>> From: Roger Quadros <rogerq@ti.com> >>> >>> This is the Ethernet driver for TI AM654 Silicon rev. 2 >>> with the ICSSG PRU Sub-system running dual-EMAC firmware. >>> [ ... ] >> >> ... >> >>> +MODULE_AUTHOR("Roger Quadros <rogerq@ti.com>"); >>> +MODULE_AUTHOR("Puranjay Mohan <p-mohan@ti.com>"); >>> +MODULE_AUTHOR("Md Danish Anwar <danishanwar@ti.com>"); >>> +MODULE_DESCRIPTION("PRUSS ICSSG Ethernet Driver"); >>> +MODULE_LICENSE("GPL"); >> >> SPDK says GPL-2.0, so perhaps this should be "GPL v2" ? >> I am getting checkpatch warning while changing GPL version. WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity") #3602: FILE: drivers/net/ethernet/ti/icssg_prueth.c:1866: +MODULE_LICENSE("GPL v2"); Should I ignore this warning and change it to "GPL v2"
On Fri, Apr 28, 2023 at 02:36:42PM +0530, Md Danish Anwar wrote: > Hi Simon. > > On 27/04/23 12:42, Md Danish Anwar wrote: > > Hi Simon, > > Thanks for the comments. > > > > On 27/04/23 00:39, Simon Horman wrote: > >> On Mon, Apr 24, 2023 at 11:02:33AM +0530, MD Danish Anwar wrote: > >>> From: Roger Quadros <rogerq@ti.com> > >>> > >>> This is the Ethernet driver for TI AM654 Silicon rev. 2 > >>> with the ICSSG PRU Sub-system running dual-EMAC firmware. > >>> > > [ ... ] > > >> > >> ... > >> > >>> +MODULE_AUTHOR("Roger Quadros <rogerq@ti.com>"); > >>> +MODULE_AUTHOR("Puranjay Mohan <p-mohan@ti.com>"); > >>> +MODULE_AUTHOR("Md Danish Anwar <danishanwar@ti.com>"); > >>> +MODULE_DESCRIPTION("PRUSS ICSSG Ethernet Driver"); > >>> +MODULE_LICENSE("GPL"); > >> > >> SPDK says GPL-2.0, so perhaps this should be "GPL v2" ? > >> > > I am getting checkpatch warning while changing GPL version. > > WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module: Cure > the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity") > #3602: FILE: drivers/net/ethernet/ti/icssg_prueth.c:1866: > +MODULE_LICENSE("GPL v2"); > > Should I ignore this warning and change it to "GPL v2" I guess that "GPL" is correct after all. Sorry for the noise.