| Message ID | 20230710053550.89160-1-danishanwar@ti.com |
|---|---|
| Headers | show |
| Series | Introduce ICSSG based ethernet Driver | expand |
On Mon, Jul 10, 2023 at 11:05:50AM +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> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: MD Danish Anwar <danishanwar@ti.com> ... > +/** > + * struct map - ICSSG Queue Map > + * @queue: Queue number > + * @pd_addr_start: Packet descriptor queue reserved memory > + * @flags: Flags > + * @special: Indicates whether this queue is a special queue or not > + */ > +struct map { > + int queue; > + u32 pd_addr_start; > + u32 flags; > + bool special; > +}; > + > +/* Hardware queue map for ICSSG */ > +const struct map hwq_map[2][ICSSG_NUM_OTHER_QUEUES] = { Should this be static? > + { > + { PORT_HI_Q_SLICE0, PORT_DESC0_HI, 0x200000, 0 }, > + { PORT_LO_Q_SLICE0, PORT_DESC0_LO, 0, 0 }, > + { HOST_HI_Q_SLICE0, HOST_DESC0_HI, 0x200000, 0 }, > + { HOST_LO_Q_SLICE0, HOST_DESC0_LO, 0, 0 }, > + { HOST_SPL_Q_SLICE0, HOST_SPPD0, 0x400000, 1 }, > + }, > + { > + { PORT_HI_Q_SLICE1, PORT_DESC1_HI, 0xa00000, 0 }, > + { PORT_LO_Q_SLICE1, PORT_DESC1_LO, 0x800000, 0 }, > + { HOST_HI_Q_SLICE1, HOST_DESC1_HI, 0xa00000, 0 }, > + { HOST_LO_Q_SLICE1, HOST_DESC1_LO, 0x800000, 0 }, > + { HOST_SPL_Q_SLICE1, HOST_SPPD1, 0xc00000, 1 }, > + }, > +}; > + > +static void icssg_config_mii_init(struct prueth_emac *emac) > +{ > + 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; > + > + 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++) { > + const struct map *mp; > + int pd_size, num_pds; > + u32 pdaddr; > + > + 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); Sparse warns that the endieness of pdword is not le32. There are also other sparse warnings added by this patch. Please look over them. > + 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); > + } > + } > +} ... > +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; ... > + 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; ndev appears to be leaked here. ... > + 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; > + if (!of_device_is_available(eth0_node)) { > + of_node_put(eth0_node); > + eth0_node = NULL; > + } > + } else if (reg == 1) { > + eth1_node = eth_node; > + if (!of_device_is_available(eth1_node)) { > + of_node_put(eth1_node); > + eth1_node = NULL; > + } > + } else { > + dev_err(dev, "port reg should be 0 or 1\n"); Should this be treated as an error and either return or goto an unwind path? > + } > + } > + > + of_node_put(eth_ports_node); > + > + /* At least one node must be present and available else we fail */ > + if (!eth0_node && !eth1_node) { Smatch warns that eth0_node and eth1_node may be uninitialised here. > + 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; > + } ...
Hi Simon On 7/11/2023 11:15 PM, Simon Horman wrote: > On Mon, Jul 10, 2023 at 11:05:50AM +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> >> Reviewed-by: Andrew Lunn <andrew@lunn.ch> >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> > > ... > >> +/** >> + * struct map - ICSSG Queue Map >> + * @queue: Queue number >> + * @pd_addr_start: Packet descriptor queue reserved memory >> + * @flags: Flags >> + * @special: Indicates whether this queue is a special queue or not >> + */ >> +struct map { >> + int queue; >> + u32 pd_addr_start; >> + u32 flags; >> + bool special; >> +}; >> + >> +/* Hardware queue map for ICSSG */ >> +const struct map hwq_map[2][ICSSG_NUM_OTHER_QUEUES] = { > > Should this be static? > Yes this can be static. I will change this to static. >> + { >> + { PORT_HI_Q_SLICE0, PORT_DESC0_HI, 0x200000, 0 }, >> + { PORT_LO_Q_SLICE0, PORT_DESC0_LO, 0, 0 }, >> + { HOST_HI_Q_SLICE0, HOST_DESC0_HI, 0x200000, 0 }, >> + { HOST_LO_Q_SLICE0, HOST_DESC0_LO, 0, 0 }, >> + { HOST_SPL_Q_SLICE0, HOST_SPPD0, 0x400000, 1 }, >> + }, >> + { >> + { PORT_HI_Q_SLICE1, PORT_DESC1_HI, 0xa00000, 0 }, >> + { PORT_LO_Q_SLICE1, PORT_DESC1_LO, 0x800000, 0 }, >> + { HOST_HI_Q_SLICE1, HOST_DESC1_HI, 0xa00000, 0 }, >> + { HOST_LO_Q_SLICE1, HOST_DESC1_LO, 0x800000, 0 }, >> + { HOST_SPL_Q_SLICE1, HOST_SPPD1, 0xc00000, 1 }, >> + }, >> +}; >> + >> +static void icssg_config_mii_init(struct prueth_emac *emac) >> +{ >> + 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; >> + >> + 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++) { >> + const struct map *mp; >> + int pd_size, num_pds; >> + u32 pdaddr; >> + >> + 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); > > Sparse warns that the endieness of pdword is not le32. I will fix this. > There are also other sparse warnings added by this patch. > Please look over them. There is one more warning for "expected restricted __le16 [usertype] rx_base_flow got restricted __le32 [usertype]". I will fix this as well. There is one more sparse warning "warning: symbol 'icssg_ethtool_ops' was not declared. Should it be static?". This should be ignored as no need to change 'icssg_ethtool_ops' to static as this is decalred in icssg_ethtool.c and used in icssg_prueth.c > >> + 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); >> + } >> + } >> +} > > ... > >> +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; > > ... > >> + 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; > > ndev appears to be leaked here. > > ... > >> + 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; >> + if (!of_device_is_available(eth0_node)) { >> + of_node_put(eth0_node); >> + eth0_node = NULL; >> + } >> + } else if (reg == 1) { >> + eth1_node = eth_node; >> + if (!of_device_is_available(eth1_node)) { >> + of_node_put(eth1_node); >> + eth1_node = NULL; >> + } >> + } else { >> + dev_err(dev, "port reg should be 0 or 1\n"); > > Should this be treated as an error and either return or goto an > unwind path? > I don't think we should error out or return to any goto label here. Here we are checking 'reg' property in all available ports. If reg=0, we assign the node to eth0_node. If reg=1, we assign the node to eth1_node. If the reg is neither 0 nor 1, we will just keep looking through other available ports, instead of returning error. We will eventually look through all available nodes. Once we come out of the for loop, we should at least have one node with reg property being either 0 or 1. If no node had reg as 0 or 1, both eth0_node and eth1_node will be NULL, then we will error out with -ENODEV error by below if check. if (!eth0_node && !eth1_node) { dev_err(dev, "neither port0 nor port1 node available\n"); return -ENODEV; } >> + } >> + } >> + >> + of_node_put(eth_ports_node); >> + >> + /* At least one node must be present and available else we fail */ >> + if (!eth0_node && !eth1_node) { > > Smatch warns that eth0_node and eth1_node may be uninitialised here. > Sure, I will initialise eth0_node and eth1_node as NULL. >> + 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; >> + } > > ... >
Hi Anwar, On Wed, Jul 12, 2023 at 05:55:57PM +0530, Anwar, Md Danish wrote: > Hi Simon > On 7/11/2023 11:15 PM, Simon Horman wrote: > > On Mon, Jul 10, 2023 at 11:05:50AM +0530, MD Danish Anwar wrote: > > > From: Roger Quadros <rogerq@ti.com> ... > > > +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++) { > > > + const struct map *mp; > > > + int pd_size, num_pds; > > > + u32 pdaddr; > > > + > > > + 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); > > > > Sparse warns that the endieness of pdword is not le32. > > I will fix this. Thanks. > > There are also other sparse warnings added by this patch. > > Please look over them. > > There is one more warning for "expected restricted __le16 [usertype] > rx_base_flow got restricted __le32 [usertype]". I will fix this as well. I haven't looked carefully through these. But for the record, this is what Sparse tells me: .../icssg_config.c:91:18: warning: symbol 'hwq_map' was not declared. Should it be static? .../icssg_config.c:189:35: warning: invalid assignment: &= .../icssg_config.c:189:35: left side has type unsigned int .../icssg_config.c:189:35: right side has type restricted __le32 .../icssg_config.c:190:35: warning: invalid assignment: |= .../icssg_config.c:190:35: left side has type unsigned int .../icssg_config.c:190:35: right side has type restricted __le32 .../icssg_config.c:225:11: warning: incorrect type in assignment (different address spaces) .../icssg_config.c:225:11: expected struct icssg_r30_cmd *p .../icssg_config.c:225:11: got void [noderef] __iomem * .../icssg_config.c:228:42: warning: incorrect type in argument 2 (different address spaces) .../icssg_config.c:228:42: expected void volatile [noderef] __iomem *addr .../icssg_config.c:228:42: got unsigned int * .../icssg_config.c:237:11: warning: incorrect type in assignment (different address spaces) .../icssg_config.c:237:11: expected struct icssg_r30_cmd const *p .../icssg_config.c:237:11: got void [noderef] __iomem * .../icssg_config.c:240:36: warning: incorrect type in argument 1 (different address spaces) .../icssg_config.c:240:36: expected void const volatile [noderef] __iomem *addr .../icssg_config.c:240:36: got unsigned int const * .../icssg_config.c:270:19: warning: incorrect type in assignment (different address spaces) .../icssg_config.c:270:19: expected struct icssg_buffer_pool_cfg *bpool_cfg .../icssg_config.c:270:19: got void [noderef] __iomem * .../icssg_config.c:289:17: warning: incorrect type in assignment (different address spaces) .../icssg_config.c:289:17: expected struct icssg_rxq_ctx *rxq_ctx .../icssg_config.c:289:17: got void [noderef] __iomem * .../icssg_config.c:297:17: warning: incorrect type in assignment (different address spaces) .../icssg_config.c:297:17: expected struct icssg_rxq_ctx *rxq_ctx .../icssg_config.c:297:17: got void [noderef] __iomem * .../icssg_config.c:325:38: warning: incorrect type in initializer (different address spaces) .../icssg_config.c:325:38: expected void *config .../icssg_config.c:325:38: got void [noderef] __iomem * .../icssg_config.c:332:19: warning: incorrect type in argument 1 (different address spaces) .../icssg_config.c:332:19: expected void volatile [noderef] __iomem * .../icssg_config.c:332:19: got void *config .../icssg_config.c:361:32: warning: incorrect type in assignment (different base types) .../icssg_config.c:361:32: expected restricted __le16 [usertype] rx_base_flow .../icssg_config.c:361:32: got restricted __le32 [usertype] .../icssg_config.c:406:11: warning: incorrect type in assignment (different address spaces) .../icssg_config.c:406:11: expected struct icssg_r30_cmd *p .../icssg_config.c:406:11: got void [noderef] __iomem * .../icssg_config.c:417:61: warning: incorrect type in argument 2 (different address spaces) .../icssg_config.c:417:61: expected void volatile [noderef] __iomem *addr .../icssg_config.c:417:61: got unsigned int * .../icssg_prueth.c:1665:9: warning: incorrect type in argument 1 (different address spaces) .../icssg_prueth.c:1665:9: expected void const * .../icssg_prueth.c:1665:9: got void [noderef] __iomem *va .../icssg_prueth.c:1665:9: warning: incorrect type in argument 1 (different address spaces) .../icssg_prueth.c:1665:9: expected void const * .../icssg_prueth.c:1665:9: got void [noderef] __iomem *va .../icssg_prueth.c:1665:9: warning: incorrect type in argument 1 (different address spaces) .../icssg_prueth.c:1665:9: expected void * .../icssg_prueth.c:1665:9: got void [noderef] __iomem *va > There is one more sparse warning "warning: symbol 'icssg_ethtool_ops' was > not declared. Should it be static?". This should be ignored as no need to > change 'icssg_ethtool_ops' to static as this is decalred in icssg_ethtool.c > and used in icssg_prueth.c I think the preferred approach there would be to declare the symbol in a header file that is available to both .c files. ... > > > + 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; > > > + if (!of_device_is_available(eth0_node)) { > > > + of_node_put(eth0_node); > > > + eth0_node = NULL; > > > + } > > > + } else if (reg == 1) { > > > + eth1_node = eth_node; > > > + if (!of_device_is_available(eth1_node)) { > > > + of_node_put(eth1_node); > > > + eth1_node = NULL; > > > + } > > > + } else { > > > + dev_err(dev, "port reg should be 0 or 1\n"); > > > > Should this be treated as an error and either return or goto an > > unwind path? > > > > I don't think we should error out or return to any goto label here. Here we > are checking 'reg' property in all available ports. If reg=0, we assign the > node to eth0_node. If reg=1, we assign the node to eth1_node. If the reg is > neither 0 nor 1, we will just keep looking through other available ports, > instead of returning error. We will eventually look through all available > nodes. > > Once we come out of the for loop, we should at least have one node with reg > property being either 0 or 1. If no node had reg as 0 or 1, both eth0_node > and eth1_node will be NULL, then we will error out with -ENODEV error by > below if check. > > if (!eth0_node && !eth1_node) { > dev_err(dev, "neither port0 nor port1 node available\n"); > return -ENODEV; > } Thanks, that makes sense to me. > > > + } > > > + } > > > + > > > + of_node_put(eth_ports_node); > > > + > > > + /* At least one node must be present and available else we fail */ > > > + if (!eth0_node && !eth1_node) { > > > > Smatch warns that eth0_node and eth1_node may be uninitialised here. > > > > Sure, I will initialise eth0_node and eth1_node as NULL. Thanks. ...
Hi Simon, On 13/07/23 2:20 pm, Simon Horman wrote: > Hi Anwar, > > On Wed, Jul 12, 2023 at 05:55:57PM +0530, Anwar, Md Danish wrote: >> Hi Simon >> On 7/11/2023 11:15 PM, Simon Horman wrote: >>> On Mon, Jul 10, 2023 at 11:05:50AM +0530, MD Danish Anwar wrote: >>>> From: Roger Quadros <rogerq@ti.com> > > ... > >>>> +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++) { >>>> + const struct map *mp; >>>> + int pd_size, num_pds; >>>> + u32 pdaddr; >>>> + >>>> + 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); >>> >>> Sparse warns that the endieness of pdword is not le32. >> >> I will fix this. > > Thanks. > >>> There are also other sparse warnings added by this patch. >>> Please look over them. >> >> There is one more warning for "expected restricted __le16 [usertype] >> rx_base_flow got restricted __le32 [usertype]". I will fix this as well. > > I haven't looked carefully through these. > But for the record, this is what Sparse tells me: > I am working on fixing all these sparse warning. I will send next revision after fixing these warning. > .../icssg_config.c:91:18: warning: symbol 'hwq_map' was not declared. Should it be static? > .../icssg_config.c:189:35: warning: invalid assignment: &= > .../icssg_config.c:189:35: left side has type unsigned int > .../icssg_config.c:189:35: right side has type restricted __le32 > .../icssg_config.c:190:35: warning: invalid assignment: |= > .../icssg_config.c:190:35: left side has type unsigned int > .../icssg_config.c:190:35: right side has type restricted __le32 > .../icssg_config.c:225:11: warning: incorrect type in assignment (different address spaces) > .../icssg_config.c:225:11: expected struct icssg_r30_cmd *p > .../icssg_config.c:225:11: got void [noderef] __iomem * > .../icssg_config.c:228:42: warning: incorrect type in argument 2 (different address spaces) > .../icssg_config.c:228:42: expected void volatile [noderef] __iomem *addr > .../icssg_config.c:228:42: got unsigned int * > .../icssg_config.c:237:11: warning: incorrect type in assignment (different address spaces) > .../icssg_config.c:237:11: expected struct icssg_r30_cmd const *p > .../icssg_config.c:237:11: got void [noderef] __iomem * > .../icssg_config.c:240:36: warning: incorrect type in argument 1 (different address spaces) > .../icssg_config.c:240:36: expected void const volatile [noderef] __iomem *addr > .../icssg_config.c:240:36: got unsigned int const * > .../icssg_config.c:270:19: warning: incorrect type in assignment (different address spaces) > .../icssg_config.c:270:19: expected struct icssg_buffer_pool_cfg *bpool_cfg > .../icssg_config.c:270:19: got void [noderef] __iomem * > .../icssg_config.c:289:17: warning: incorrect type in assignment (different address spaces) > .../icssg_config.c:289:17: expected struct icssg_rxq_ctx *rxq_ctx > .../icssg_config.c:289:17: got void [noderef] __iomem * > .../icssg_config.c:297:17: warning: incorrect type in assignment (different address spaces) > .../icssg_config.c:297:17: expected struct icssg_rxq_ctx *rxq_ctx > .../icssg_config.c:297:17: got void [noderef] __iomem * > .../icssg_config.c:325:38: warning: incorrect type in initializer (different address spaces) > .../icssg_config.c:325:38: expected void *config > .../icssg_config.c:325:38: got void [noderef] __iomem * > .../icssg_config.c:332:19: warning: incorrect type in argument 1 (different address spaces) > .../icssg_config.c:332:19: expected void volatile [noderef] __iomem * > .../icssg_config.c:332:19: got void *config > .../icssg_config.c:361:32: warning: incorrect type in assignment (different base types) > .../icssg_config.c:361:32: expected restricted __le16 [usertype] rx_base_flow > .../icssg_config.c:361:32: got restricted __le32 [usertype] > .../icssg_config.c:406:11: warning: incorrect type in assignment (different address spaces) > .../icssg_config.c:406:11: expected struct icssg_r30_cmd *p > .../icssg_config.c:406:11: got void [noderef] __iomem * > .../icssg_config.c:417:61: warning: incorrect type in argument 2 (different address spaces) > .../icssg_config.c:417:61: expected void volatile [noderef] __iomem *addr > .../icssg_config.c:417:61: got unsigned int * > .../icssg_prueth.c:1665:9: warning: incorrect type in argument 1 (different address spaces) > .../icssg_prueth.c:1665:9: expected void const * > .../icssg_prueth.c:1665:9: got void [noderef] __iomem *va > .../icssg_prueth.c:1665:9: warning: incorrect type in argument 1 (different address spaces) > .../icssg_prueth.c:1665:9: expected void const * > .../icssg_prueth.c:1665:9: got void [noderef] __iomem *va > .../icssg_prueth.c:1665:9: warning: incorrect type in argument 1 (different address spaces) > .../icssg_prueth.c:1665:9: expected void * > .../icssg_prueth.c:1665:9: got void [noderef] __iomem *va > >> There is one more sparse warning "warning: symbol 'icssg_ethtool_ops' was >> not declared. Should it be static?". This should be ignored as no need to >> change 'icssg_ethtool_ops' to static as this is decalred in icssg_ethtool.c >> and used in icssg_prueth.c > > I think the preferred approach there would be to declare the symbol > in a header file that is available to both .c files. > Sure. I will keep the declaration in a icssg_prueth.h. > ... > >>>> + 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; >>>> + if (!of_device_is_available(eth0_node)) { >>>> + of_node_put(eth0_node); >>>> + eth0_node = NULL; >>>> + } >>>> + } else if (reg == 1) { >>>> + eth1_node = eth_node; >>>> + if (!of_device_is_available(eth1_node)) { >>>> + of_node_put(eth1_node); >>>> + eth1_node = NULL; >>>> + } >>>> + } else { >>>> + dev_err(dev, "port reg should be 0 or 1\n"); >>> >>> Should this be treated as an error and either return or goto an >>> unwind path? >>> >> >> I don't think we should error out or return to any goto label here. Here we >> are checking 'reg' property in all available ports. If reg=0, we assign the >> node to eth0_node. If reg=1, we assign the node to eth1_node. If the reg is >> neither 0 nor 1, we will just keep looking through other available ports, >> instead of returning error. We will eventually look through all available >> nodes. >> >> Once we come out of the for loop, we should at least have one node with reg >> property being either 0 or 1. If no node had reg as 0 or 1, both eth0_node >> and eth1_node will be NULL, then we will error out with -ENODEV error by >> below if check. >> >> if (!eth0_node && !eth1_node) { >> dev_err(dev, "neither port0 nor port1 node available\n"); >> return -ENODEV; >> } > > Thanks, that makes sense to me. > >>>> + } >>>> + } >>>> + >>>> + of_node_put(eth_ports_node); >>>> + >>>> + /* At least one node must be present and available else we fail */ >>>> + if (!eth0_node && !eth1_node) { >>> >>> Smatch warns that eth0_node and eth1_node may be uninitialised here. >>> >> >> Sure, I will initialise eth0_node and eth1_node as NULL. > > Thanks. > > ... I will fix all the sparse and smatch warning and send next revision.
Hi Danish, On Thu, Jul 13, 2023 at 05:11:12PM +0530, Md Danish Anwar wrote: > Hi Simon, > > On 13/07/23 2:20 pm, Simon Horman wrote: > > Hi Anwar, > > > > On Wed, Jul 12, 2023 at 05:55:57PM +0530, Anwar, Md Danish wrote: > >> Hi Simon > >> On 7/11/2023 11:15 PM, Simon Horman wrote: > >>> On Mon, Jul 10, 2023 at 11:05:50AM +0530, MD Danish Anwar wrote: > >>>> From: Roger Quadros <rogerq@ti.com> ... > >>> Smatch warns that eth0_node and eth1_node may be uninitialised here. > >>> > >> > >> Sure, I will initialise eth0_node and eth1_node as NULL. > > > > Thanks. > > > > ... > > I will fix all the sparse and smatch warning and send next revision. Perfect, thanks!