| Message ID | 20220406094358.7895-1-p-mohan@ti.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | PRUSS Remoteproc, Platform APIS, and Ethernet Driver | expand |
> +static int emac_set_link_ksettings(struct net_device *ndev, > + const struct ethtool_link_ksettings *ecmd) > +{ > + struct prueth_emac *emac = netdev_priv(ndev); > + > + if (!emac->phydev || phy_is_pseudo_fixed_link(emac->phydev)) > + return -EOPNOTSUPP; > + > + return phy_ethtool_ksettings_set(emac->phydev, ecmd); > +} > + > +static int emac_get_eee(struct net_device *ndev, struct ethtool_eee *edata) > +{ > + struct prueth_emac *emac = netdev_priv(ndev); > + > + if (!emac->phydev || phy_is_pseudo_fixed_link(emac->phydev)) > + return -EOPNOTSUPP; > + > + return phy_ethtool_get_eee(emac->phydev, edata); > +} Why do you need the phy_is_pseudo_fixed_link() calls here? > +/* called back by PHY layer if there is change in link state of hw port*/ > +static void emac_adjust_link(struct net_device *ndev) > +{ ... > + if (emac->link) { > + /* link ON */ > + netif_carrier_on(ndev); > + /* reactivate the transmit queue */ > + netif_tx_wake_all_queues(ndev); > + } else { > + /* link OFF */ > + netif_carrier_off(ndev); > + netif_tx_stop_all_queues(ndev); > + } phylib should of set the carrier for you. > + * emac_ndo_open - EMAC device open > + * @ndev: network adapter device > + * > + * Called when system wants to start the interface. > + * > + * Returns 0 for a successful open, or appropriate error code > + */ > +static int emac_ndo_open(struct net_device *ndev) > +{ > + struct prueth_emac *emac = netdev_priv(ndev); > + int ret, i, num_data_chn = emac->tx_ch_num; > + struct prueth *prueth = emac->prueth; > + int slice = prueth_emac_slice(emac); > + struct device *dev = prueth->dev; > + int max_rx_flows; > + int rx_flow; > + > + /* clear SMEM and MSMC settings for all slices */ > + if (!prueth->emacs_initialized) { > + memset_io(prueth->msmcram.va, 0, prueth->msmcram.size); > + memset_io(prueth->shram.va, 0, ICSSG_CONFIG_OFFSET_SLICE1 * PRUETH_NUM_MACS); > + } > + > + /* set h/w MAC as user might have re-configured */ > + ether_addr_copy(emac->mac_addr, ndev->dev_addr); > + > + icssg_class_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr); > + icssg_ft1_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr); > + > + icssg_class_default(prueth->miig_rt, slice, 0); > + > + netif_carrier_off(ndev); phylib should take care of this. > + > + /* Notify the stack of the actual queue counts. */ > + ret = netif_set_real_num_tx_queues(ndev, num_data_chn); > + if (ret) { > + dev_err(dev, "cannot set real number of tx queues\n"); > + return ret; > + } > + > + init_completion(&emac->cmd_complete); > + ret = prueth_init_tx_chns(emac); > + if (ret) { > + dev_err(dev, "failed to init tx channel: %d\n", ret); > + return ret; > + } > + > + max_rx_flows = PRUETH_MAX_RX_FLOWS; > + ret = prueth_init_rx_chns(emac, &emac->rx_chns, "rx", > + max_rx_flows, PRUETH_MAX_RX_DESC); > + if (ret) { > + dev_err(dev, "failed to init rx channel: %d\n", ret); > + goto cleanup_tx; > + } > + > + ret = prueth_ndev_add_tx_napi(emac); > + if (ret) > + goto cleanup_rx; > + > + /* we use only the highest priority flow for now i.e. @irq[3] */ > + rx_flow = PRUETH_RX_FLOW_DATA; > + ret = request_irq(emac->rx_chns.irq[rx_flow], prueth_rx_irq, > + IRQF_TRIGGER_HIGH, dev_name(dev), emac); > + if (ret) { > + dev_err(dev, "unable to request RX IRQ\n"); > + goto cleanup_napi; > + } > + > + /* reset and start PRU firmware */ > + ret = prueth_emac_start(prueth, emac); > + if (ret) > + goto free_rx_irq; > + > + /* Prepare RX */ > + ret = prueth_prepare_rx_chan(emac, &emac->rx_chns, PRUETH_MAX_PKT_SIZE); > + if (ret) > + goto stop; > + > + ret = k3_udma_glue_enable_rx_chn(emac->rx_chns.rx_chn); > + if (ret) > + goto reset_rx_chn; > + > + for (i = 0; i < emac->tx_ch_num; i++) { > + ret = k3_udma_glue_enable_tx_chn(emac->tx_chns[i].tx_chn); > + if (ret) > + goto reset_tx_chan; > + } > + > + /* Enable NAPI in Tx and Rx direction */ > + for (i = 0; i < emac->tx_ch_num; i++) > + napi_enable(&emac->tx_chns[i].napi_tx); > + napi_enable(&emac->napi_rx); > + > + emac_phy_connect(emac); Why don't you check the error code? > +static int prueth_config_rgmiidelay(struct prueth *prueth, > + struct device_node *eth_np, > + phy_interface_t phy_if) > +{ > + struct device *dev = prueth->dev; > + struct regmap *ctrl_mmr; > + u32 rgmii_tx_id = 0; > + u32 icssgctrl_reg; > + > + if (!phy_interface_mode_is_rgmii(phy_if)) > + return 0; > + > + ctrl_mmr = syscon_regmap_lookup_by_phandle(eth_np, "ti,syscon-rgmii-delay"); > + if (IS_ERR(ctrl_mmr)) { > + dev_err(dev, "couldn't get ti,syscon-rgmii-delay\n"); > + return -ENODEV; > + } > + > + if (of_property_read_u32_index(eth_np, "ti,syscon-rgmii-delay", 1, > + &icssgctrl_reg)) { > + dev_err(dev, "couldn't get ti,rgmii-delay reg. offset\n"); > + return -ENODEV; > + } > + > + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || > + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) > + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; > + > + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); Do you need to do a units conversion here, or does the register already take pico seconds? Andrew
> +static int emac_phy_connect(struct prueth_emac *emac) > +{ > + struct prueth *prueth = emac->prueth; > + > + /* connect PHY */ > + emac->phydev = of_phy_connect(emac->ndev, emac->phy_node, > + &emac_adjust_link, 0, emac->phy_if); > +static int prueth_config_rgmiidelay(struct prueth *prueth, > + struct device_node *eth_np, > + phy_interface_t phy_if) > +{ > + struct device *dev = prueth->dev; > + struct regmap *ctrl_mmr; > + u32 rgmii_tx_id = 0; > + u32 icssgctrl_reg; > + > + if (!phy_interface_mode_is_rgmii(phy_if)) > + return 0; > + > + ctrl_mmr = syscon_regmap_lookup_by_phandle(eth_np, "ti,syscon-rgmii-delay"); > + if (IS_ERR(ctrl_mmr)) { > + dev_err(dev, "couldn't get ti,syscon-rgmii-delay\n"); > + return -ENODEV; > + } > + > + if (of_property_read_u32_index(eth_np, "ti,syscon-rgmii-delay", 1, > + &icssgctrl_reg)) { > + dev_err(dev, "couldn't get ti,rgmii-delay reg. offset\n"); > + return -ENODEV; > + } > + > + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || > + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) > + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; > + > + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); > + > + return 0; > +} > O.K, so this does not do what i initially thought it was doing. I was thinking it was to fine tune the delay, ti,syscon-rgmii-delay would be a small pico second value to allow the 2ns delay to be tuned to the board. But now i think this is actually inserting the full 2ns delay? The problem is, you also pass phy_if to of_phy_connect() so the PHY will also insert the delay if requested. So you end up with double delays for rgmii_id and rgmii_txid. The general recommendation is that the PHY inserts the delay, based on phy-mode. The MAC does not add a delay, so i suggest you always write 0 here, just to ensure the system is in a deterministic state, and the bootloader and not being messing around with things. Andrew
> +config TI_ICSSG_PRUETH > + tristate "TI Gigabit PRU Ethernet driver" > + select TI_DAVINCI_MDIO > + I don't see a dependency on TI_DAVINCI_MDIO in the code. All you need is an MDIO bus so that your phy-handle has somewhere to point. But that could be a GPIO bit banger. What i do think is missing here is a dependency on PHYLIB. If possible, it would be good to also have it compile when COMPILE_TEST is set. Andrew
+ Roger, Grygorii On 06/04/22 19:43, Andrew Lunn wrote: >> +static int emac_set_link_ksettings(struct net_device *ndev, >> + const struct ethtool_link_ksettings *ecmd) >> +{ >> + struct prueth_emac *emac = netdev_priv(ndev); >> + >> + if (!emac->phydev || phy_is_pseudo_fixed_link(emac->phydev)) >> + return -EOPNOTSUPP; >> + >> + return phy_ethtool_ksettings_set(emac->phydev, ecmd); >> +} >> + >> +static int emac_get_eee(struct net_device *ndev, struct ethtool_eee *edata) >> +{ >> + struct prueth_emac *emac = netdev_priv(ndev); >> + >> + if (!emac->phydev || phy_is_pseudo_fixed_link(emac->phydev)) >> + return -EOPNOTSUPP; >> + >> + return phy_ethtool_get_eee(emac->phydev, edata); >> +} > > Why do you need the phy_is_pseudo_fixed_link() calls here? > >> +/* called back by PHY layer if there is change in link state of hw port*/ >> +static void emac_adjust_link(struct net_device *ndev) >> +{ > > ... > >> + if (emac->link) { >> + /* link ON */ >> + netif_carrier_on(ndev); >> + /* reactivate the transmit queue */ >> + netif_tx_wake_all_queues(ndev); >> + } else { >> + /* link OFF */ >> + netif_carrier_off(ndev); >> + netif_tx_stop_all_queues(ndev); >> + } > > phylib should of set the carrier for you. > >> + * emac_ndo_open - EMAC device open >> + * @ndev: network adapter device >> + * >> + * Called when system wants to start the interface. >> + * >> + * Returns 0 for a successful open, or appropriate error code >> + */ >> +static int emac_ndo_open(struct net_device *ndev) >> +{ >> + struct prueth_emac *emac = netdev_priv(ndev); >> + int ret, i, num_data_chn = emac->tx_ch_num; >> + struct prueth *prueth = emac->prueth; >> + int slice = prueth_emac_slice(emac); >> + struct device *dev = prueth->dev; >> + int max_rx_flows; >> + int rx_flow; >> + >> + /* clear SMEM and MSMC settings for all slices */ >> + if (!prueth->emacs_initialized) { >> + memset_io(prueth->msmcram.va, 0, prueth->msmcram.size); >> + memset_io(prueth->shram.va, 0, ICSSG_CONFIG_OFFSET_SLICE1 * PRUETH_NUM_MACS); >> + } >> + >> + /* set h/w MAC as user might have re-configured */ >> + ether_addr_copy(emac->mac_addr, ndev->dev_addr); >> + >> + icssg_class_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr); >> + icssg_ft1_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr); >> + >> + icssg_class_default(prueth->miig_rt, slice, 0); >> + >> + netif_carrier_off(ndev); > > phylib should take care of this. > >> + >> + /* Notify the stack of the actual queue counts. */ >> + ret = netif_set_real_num_tx_queues(ndev, num_data_chn); >> + if (ret) { >> + dev_err(dev, "cannot set real number of tx queues\n"); >> + return ret; >> + } >> + >> + init_completion(&emac->cmd_complete); >> + ret = prueth_init_tx_chns(emac); >> + if (ret) { >> + dev_err(dev, "failed to init tx channel: %d\n", ret); >> + return ret; >> + } >> + >> + max_rx_flows = PRUETH_MAX_RX_FLOWS; >> + ret = prueth_init_rx_chns(emac, &emac->rx_chns, "rx", >> + max_rx_flows, PRUETH_MAX_RX_DESC); >> + if (ret) { >> + dev_err(dev, "failed to init rx channel: %d\n", ret); >> + goto cleanup_tx; >> + } >> + >> + ret = prueth_ndev_add_tx_napi(emac); >> + if (ret) >> + goto cleanup_rx; >> + >> + /* we use only the highest priority flow for now i.e. @irq[3] */ >> + rx_flow = PRUETH_RX_FLOW_DATA; >> + ret = request_irq(emac->rx_chns.irq[rx_flow], prueth_rx_irq, >> + IRQF_TRIGGER_HIGH, dev_name(dev), emac); >> + if (ret) { >> + dev_err(dev, "unable to request RX IRQ\n"); >> + goto cleanup_napi; >> + } >> + >> + /* reset and start PRU firmware */ >> + ret = prueth_emac_start(prueth, emac); >> + if (ret) >> + goto free_rx_irq; >> + >> + /* Prepare RX */ >> + ret = prueth_prepare_rx_chan(emac, &emac->rx_chns, PRUETH_MAX_PKT_SIZE); >> + if (ret) >> + goto stop; >> + >> + ret = k3_udma_glue_enable_rx_chn(emac->rx_chns.rx_chn); >> + if (ret) >> + goto reset_rx_chn; >> + >> + for (i = 0; i < emac->tx_ch_num; i++) { >> + ret = k3_udma_glue_enable_tx_chn(emac->tx_chns[i].tx_chn); >> + if (ret) >> + goto reset_tx_chan; >> + } >> + >> + /* Enable NAPI in Tx and Rx direction */ >> + for (i = 0; i < emac->tx_ch_num; i++) >> + napi_enable(&emac->tx_chns[i].napi_tx); >> + napi_enable(&emac->napi_rx); >> + >> + emac_phy_connect(emac); > > Why don't you check the error code? > >> +static int prueth_config_rgmiidelay(struct prueth *prueth, >> + struct device_node *eth_np, >> + phy_interface_t phy_if) >> +{ >> + struct device *dev = prueth->dev; >> + struct regmap *ctrl_mmr; >> + u32 rgmii_tx_id = 0; >> + u32 icssgctrl_reg; >> + >> + if (!phy_interface_mode_is_rgmii(phy_if)) >> + return 0; >> + >> + ctrl_mmr = syscon_regmap_lookup_by_phandle(eth_np, "ti,syscon-rgmii-delay"); >> + if (IS_ERR(ctrl_mmr)) { >> + dev_err(dev, "couldn't get ti,syscon-rgmii-delay\n"); >> + return -ENODEV; >> + } >> + >> + if (of_property_read_u32_index(eth_np, "ti,syscon-rgmii-delay", 1, >> + &icssgctrl_reg)) { >> + dev_err(dev, "couldn't get ti,rgmii-delay reg. offset\n"); >> + return -ENODEV; >> + } >> + >> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >> + >> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); > > Do you need to do a units conversion here, or does the register > already take pico seconds? > > Andrew
+ Roger, Grygorii On 07/04/22 00:07, Andrew Lunn wrote: >> +static int emac_phy_connect(struct prueth_emac *emac) >> +{ >> + struct prueth *prueth = emac->prueth; >> + >> + /* connect PHY */ >> + emac->phydev = of_phy_connect(emac->ndev, emac->phy_node, >> + &emac_adjust_link, 0, emac->phy_if); > >> +static int prueth_config_rgmiidelay(struct prueth *prueth, >> + struct device_node *eth_np, >> + phy_interface_t phy_if) >> +{ >> + struct device *dev = prueth->dev; >> + struct regmap *ctrl_mmr; >> + u32 rgmii_tx_id = 0; >> + u32 icssgctrl_reg; >> + >> + if (!phy_interface_mode_is_rgmii(phy_if)) >> + return 0; >> + >> + ctrl_mmr = syscon_regmap_lookup_by_phandle(eth_np, "ti,syscon-rgmii-delay"); >> + if (IS_ERR(ctrl_mmr)) { >> + dev_err(dev, "couldn't get ti,syscon-rgmii-delay\n"); >> + return -ENODEV; >> + } >> + >> + if (of_property_read_u32_index(eth_np, "ti,syscon-rgmii-delay", 1, >> + &icssgctrl_reg)) { >> + dev_err(dev, "couldn't get ti,rgmii-delay reg. offset\n"); >> + return -ENODEV; >> + } >> + >> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >> + >> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); >> + >> + return 0; >> +} >> > > O.K, so this does not do what i initially thought it was doing. I was > thinking it was to fine tune the delay, ti,syscon-rgmii-delay would be > a small pico second value to allow the 2ns delay to be tuned to the > board. > > But now i think this is actually inserting the full 2ns delay? > > The problem is, you also pass phy_if to of_phy_connect() so the PHY > will also insert the delay if requested. So you end up with double > delays for rgmii_id and rgmii_txid. > > The general recommendation is that the PHY inserts the delay, based on > phy-mode. The MAC does not add a delay, so i suggest you always write > 0 here, just to ensure the system is in a deterministic state, and the > bootloader and not being messing around with things. > > Andrew
+Roger, Grygorii On 07/04/22 00:12, Andrew Lunn wrote: >> +config TI_ICSSG_PRUETH >> + tristate "TI Gigabit PRU Ethernet driver" >> + select TI_DAVINCI_MDIO >> + > > I don't see a dependency on TI_DAVINCI_MDIO in the code. All you need > is an MDIO bus so that your phy-handle has somewhere to point. But that > could be a GPIO bit banger. > > What i do think is missing here is a dependency on PHYLIB. > > If possible, it would be good to also have it compile when > COMPILE_TEST is set. > > Andrew
On 12/04/2022 12:45, Puranjay Mohan wrote: > + Roger, Grygorii > > On 07/04/22 00:07, Andrew Lunn wrote: >>> +static int emac_phy_connect(struct prueth_emac *emac) >>> +{ >>> + struct prueth *prueth = emac->prueth; >>> + >>> + /* connect PHY */ >>> + emac->phydev = of_phy_connect(emac->ndev, emac->phy_node, >>> + &emac_adjust_link, 0, emac->phy_if); >> >>> +static int prueth_config_rgmiidelay(struct prueth *prueth, >>> + struct device_node *eth_np, >>> + phy_interface_t phy_if) >>> +{ >>> + struct device *dev = prueth->dev; >>> + struct regmap *ctrl_mmr; >>> + u32 rgmii_tx_id = 0; >>> + u32 icssgctrl_reg; >>> + >>> + if (!phy_interface_mode_is_rgmii(phy_if)) >>> + return 0; >>> + >>> + ctrl_mmr = syscon_regmap_lookup_by_phandle(eth_np, "ti,syscon-rgmii-delay"); >>> + if (IS_ERR(ctrl_mmr)) { >>> + dev_err(dev, "couldn't get ti,syscon-rgmii-delay\n"); >>> + return -ENODEV; >>> + } >>> + >>> + if (of_property_read_u32_index(eth_np, "ti,syscon-rgmii-delay", 1, >>> + &icssgctrl_reg)) { >>> + dev_err(dev, "couldn't get ti,rgmii-delay reg. offset\n"); >>> + return -ENODEV; >>> + } >>> + >>> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >>> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >>> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >>> + >>> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); >>> + >>> + return 0; >>> +} >>> >> >> O.K, so this does not do what i initially thought it was doing. I was >> thinking it was to fine tune the delay, ti,syscon-rgmii-delay would be >> a small pico second value to allow the 2ns delay to be tuned to the >> board. >> >> But now i think this is actually inserting the full 2ns delay? >> >> The problem is, you also pass phy_if to of_phy_connect() so the PHY >> will also insert the delay if requested. So you end up with double >> delays for rgmii_id and rgmii_txid. It's misunderstanding here. The bit field name in TRM is RGMII0_ID_MODE and meaning: 0h - Internal transmit delay is enabled 1h - Internal transmit delay is not enabled. So here internal delay will be disabled for RGMII_ID/RGMII_TXID. >> >> The general recommendation is that the PHY inserts the delay, based on >> phy-mode. The MAC does not add a delay, so i suggest you always write >> 0 here, just to ensure the system is in a deterministic state, and the >> bootloader and not being messing around with things. >> >> Andrew
> > > > + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || > > > > + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) > > > > + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; > > > > + > > > > + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); > > > > + > > > > + return 0; > > > > +} > > > > > > > > > > O.K, so this does not do what i initially thought it was doing. I was > > > thinking it was to fine tune the delay, ti,syscon-rgmii-delay would be > > > a small pico second value to allow the 2ns delay to be tuned to the > > > board. > > > > > > But now i think this is actually inserting the full 2ns delay? > > > > > > The problem is, you also pass phy_if to of_phy_connect() so the PHY > > > will also insert the delay if requested. So you end up with double > > > delays for rgmii_id and rgmii_txid. > > It's misunderstanding here. The bit field name in TRM is RGMII0_ID_MODE > and meaning: > 0h - Internal transmit delay is enabled > 1h - Internal transmit delay is not enabled. > > So here internal delay will be disabled for RGMII_ID/RGMII_TXID. And enabled for the others? Why don't you always disable the delays and let the PHY do it? That is what pretty much every other MAC/PHY combination does. Andrew
On 12/04/2022 12:46, Puranjay Mohan wrote: > +Roger, Grygorii > > On 07/04/22 00:12, Andrew Lunn wrote: >>> +config TI_ICSSG_PRUETH >>> + tristate "TI Gigabit PRU Ethernet driver" >>> + select TI_DAVINCI_MDIO >>> + >> >> I don't see a dependency on TI_DAVINCI_MDIO in the code. All you need >> is an MDIO bus so that your phy-handle has somewhere to point. But that >> could be a GPIO bit banger. >> >> What i do think is missing here is a dependency on PHYLIB. That is correct. >> >> If possible, it would be good to also have it compile when >> COMPILE_TEST is set. >> Yes, that is a good idea. cheers, -roger
Hi, On 12/04/2022 12:42, Puranjay Mohan wrote: > + Roger, Grygorii > > On 06/04/22 19:43, Andrew Lunn wrote: >>> +static int emac_set_link_ksettings(struct net_device *ndev, >>> + const struct ethtool_link_ksettings *ecmd) >>> +{ >>> + struct prueth_emac *emac = netdev_priv(ndev); >>> + >>> + if (!emac->phydev || phy_is_pseudo_fixed_link(emac->phydev)) >>> + return -EOPNOTSUPP; >>> + >>> + return phy_ethtool_ksettings_set(emac->phydev, ecmd); >>> +} >>> + >>> +static int emac_get_eee(struct net_device *ndev, struct ethtool_eee *edata) >>> +{ >>> + struct prueth_emac *emac = netdev_priv(ndev); >>> + >>> + if (!emac->phydev || phy_is_pseudo_fixed_link(emac->phydev)) >>> + return -EOPNOTSUPP; >>> + >>> + return phy_ethtool_get_eee(emac->phydev, edata); >>> +} >> >> Why do you need the phy_is_pseudo_fixed_link() calls here? I think this is left over code from early days. It can be removed. >> >>> +/* called back by PHY layer if there is change in link state of hw port*/ >>> +static void emac_adjust_link(struct net_device *ndev) >>> +{ >> >> ... >> >>> + if (emac->link) { >>> + /* link ON */ >>> + netif_carrier_on(ndev); >>> + /* reactivate the transmit queue */ >>> + netif_tx_wake_all_queues(ndev); >>> + } else { >>> + /* link OFF */ >>> + netif_carrier_off(ndev); >>> + netif_tx_stop_all_queues(ndev); >>> + } >> >> phylib should of set the carrier for you. >> >>> + * emac_ndo_open - EMAC device open >>> + * @ndev: network adapter device >>> + * >>> + * Called when system wants to start the interface. >>> + * >>> + * Returns 0 for a successful open, or appropriate error code >>> + */ >>> +static int emac_ndo_open(struct net_device *ndev) >>> +{ >>> + struct prueth_emac *emac = netdev_priv(ndev); >>> + int ret, i, num_data_chn = emac->tx_ch_num; >>> + struct prueth *prueth = emac->prueth; >>> + int slice = prueth_emac_slice(emac); >>> + struct device *dev = prueth->dev; >>> + int max_rx_flows; >>> + int rx_flow; >>> + >>> + /* clear SMEM and MSMC settings for all slices */ >>> + if (!prueth->emacs_initialized) { >>> + memset_io(prueth->msmcram.va, 0, prueth->msmcram.size); >>> + memset_io(prueth->shram.va, 0, ICSSG_CONFIG_OFFSET_SLICE1 * PRUETH_NUM_MACS); >>> + } >>> + >>> + /* set h/w MAC as user might have re-configured */ >>> + ether_addr_copy(emac->mac_addr, ndev->dev_addr); >>> + >>> + icssg_class_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr); >>> + icssg_ft1_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr); >>> + >>> + icssg_class_default(prueth->miig_rt, slice, 0); >>> + >>> + netif_carrier_off(ndev); >> >> phylib should take care of this. >> >>> + >>> + /* Notify the stack of the actual queue counts. */ >>> + ret = netif_set_real_num_tx_queues(ndev, num_data_chn); >>> + if (ret) { >>> + dev_err(dev, "cannot set real number of tx queues\n"); >>> + return ret; >>> + } >>> + >>> + init_completion(&emac->cmd_complete); >>> + ret = prueth_init_tx_chns(emac); >>> + if (ret) { >>> + dev_err(dev, "failed to init tx channel: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + max_rx_flows = PRUETH_MAX_RX_FLOWS; >>> + ret = prueth_init_rx_chns(emac, &emac->rx_chns, "rx", >>> + max_rx_flows, PRUETH_MAX_RX_DESC); >>> + if (ret) { >>> + dev_err(dev, "failed to init rx channel: %d\n", ret); >>> + goto cleanup_tx; >>> + } >>> + >>> + ret = prueth_ndev_add_tx_napi(emac); >>> + if (ret) >>> + goto cleanup_rx; >>> + >>> + /* we use only the highest priority flow for now i.e. @irq[3] */ >>> + rx_flow = PRUETH_RX_FLOW_DATA; >>> + ret = request_irq(emac->rx_chns.irq[rx_flow], prueth_rx_irq, >>> + IRQF_TRIGGER_HIGH, dev_name(dev), emac); >>> + if (ret) { >>> + dev_err(dev, "unable to request RX IRQ\n"); >>> + goto cleanup_napi; >>> + } >>> + >>> + /* reset and start PRU firmware */ >>> + ret = prueth_emac_start(prueth, emac); >>> + if (ret) >>> + goto free_rx_irq; >>> + >>> + /* Prepare RX */ >>> + ret = prueth_prepare_rx_chan(emac, &emac->rx_chns, PRUETH_MAX_PKT_SIZE); >>> + if (ret) >>> + goto stop; >>> + >>> + ret = k3_udma_glue_enable_rx_chn(emac->rx_chns.rx_chn); >>> + if (ret) >>> + goto reset_rx_chn; >>> + >>> + for (i = 0; i < emac->tx_ch_num; i++) { >>> + ret = k3_udma_glue_enable_tx_chn(emac->tx_chns[i].tx_chn); >>> + if (ret) >>> + goto reset_tx_chan; >>> + } >>> + >>> + /* Enable NAPI in Tx and Rx direction */ >>> + for (i = 0; i < emac->tx_ch_num; i++) >>> + napi_enable(&emac->tx_chns[i].napi_tx); >>> + napi_enable(&emac->napi_rx); >>> + >>> + emac_phy_connect(emac); >> >> Why don't you check the error code? >> >>> +static int prueth_config_rgmiidelay(struct prueth *prueth, >>> + struct device_node *eth_np, >>> + phy_interface_t phy_if) >>> +{ >>> + struct device *dev = prueth->dev; >>> + struct regmap *ctrl_mmr; >>> + u32 rgmii_tx_id = 0; >>> + u32 icssgctrl_reg; >>> + >>> + if (!phy_interface_mode_is_rgmii(phy_if)) >>> + return 0; >>> + >>> + ctrl_mmr = syscon_regmap_lookup_by_phandle(eth_np, "ti,syscon-rgmii-delay"); >>> + if (IS_ERR(ctrl_mmr)) { >>> + dev_err(dev, "couldn't get ti,syscon-rgmii-delay\n"); >>> + return -ENODEV; >>> + } >>> + >>> + if (of_property_read_u32_index(eth_np, "ti,syscon-rgmii-delay", 1, >>> + &icssgctrl_reg)) { >>> + dev_err(dev, "couldn't get ti,rgmii-delay reg. offset\n"); >>> + return -ENODEV; >>> + } >>> + >>> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >>> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >>> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >>> + >>> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); >> >> Do you need to do a units conversion here, or does the register >> already take pico seconds? I think Grygorii already answered this. It is just a fixed delay enable/disable bit. cheers, -roger
Hi Everyone, This series includes three separate patch series which are supposed to be applied to three different trees, remoteproc, soc, and net respectively These are the three series in sequence:- 1. Introduce PRU remoteproc consumer API (5 Patches) 2. Introduce PRU platform consumer API (6 Patches) 3. Introduce ICSSG based ethernet Driver (2 Patches) I am sending all three series together as RFC because 2nd depends on 1st and 3rd depends on both 1st and 2nd. Once I have received the comments on this, I will send these series separately to their respective trees. The 1st series is the v3 of [1], which had some checkpatch errors, that have been corrected in v3(this). The 2nd series is the v2 of [2], It has no functional changes. [1] https://patchwork.kernel.org/project/linux-remoteproc/cover/20201216165239.2744-1-grzegorz.jaszczyk@linaro.org/ [2] https://patchwork.kernel.org/project/linux-remoteproc/cover/20201211184811.6490-1-grzegorz.jaszczyk@linaro.org/ Now, I am adding the cover letters and details about the individual series 1. Introduce PRU remoteproc consumer API The Programmable Real-Time Unit and Industrial Communication Subsystem (PRU-ICSS or simply PRUSS) on various TI SoCs consists of dual 32-bit RISC cores (Programmable Real-Time Units, or PRUs) for program execution. There are 3 foundation components for PRUSS subsystem: the PRUSS platform driver, the PRUSS INTC driver and the PRUSS remoteproc driver. All were already merged and can be found under: 1) drivers/soc/ti/pruss.c Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml 2) drivers/irqchip/irq-pruss-intc.c Documentation/devicetree/bindings/interrupt-controller/ti,pruss-intc.yaml 3) drivers/remoteproc/pru_rproc.c Documentation/devicetree/bindings/remoteproc/ti,pru-rproc.yaml The programmable nature of the PRUs provide flexibility to implement custom peripheral interfaces, fast real-time responses, or specialized data handling. Example of a PRU consumer drivers will be: - Software UART over PRUSS - PRU-ICSS Ethernet EMAC In order to make usage of common PRU resources and allow the consumer drivers to configure the PRU hardware for specific usage the PRU API is introduced. Roger Quadros (1): remoteproc: pru: Add pru_rproc_set_ctable() function Suman Anna (2): dt-bindings: remoteproc: Add PRU consumer bindings remoteproc: pru: Make sysfs entries read-only for PRU client driven boots Tero Kristo (2): remoteproc: pru: Add APIs to get and put the PRU cores remoteproc: pru: Configure firmware based on client setup 2. Introduce PRU platform consumer API The Programmable Real-Time Unit and Industrial Communication Subsystem (PRU-ICSS or simply PRUSS) on various TI SoCs consists of dual 32-bit RISC cores (Programmable Real-Time Units, or PRUs) for program execution. There are 3 foundation components for TI PRUSS subsystem: the PRUSS platform driver, the PRUSS INTC driver and the PRUSS remoteproc driver. The programmable nature of the PRUs provide flexibility to implement custom peripheral interfaces, fast real-time responses, or specialized data handling. Example of a PRU consumer drivers will be: - Software UART over PRUSS - PRU-ICSS Ethernet EMAC In order to make usage of common PRU resources and allow the consumer drivers to configure the PRU hardware for specific usage the PRU API is introduced. Andrew F. Davis (1): soc: ti: pruss: Add pruss_{request,release}_mem_region() API Suman Anna (3): soc: ti: pruss: Add pruss_cfg_read()/update() API soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and XFR soc: ti: pruss: Add helper function to enable OCP master ports Tero Kristo (2): soc: ti: pruss: Add pruss_get()/put() API soc: ti: pruss: Add helper functions to get/set PRUSS_CFG_GPMUX 3. Introduce ICSSG based ethernet Driver 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. The subsystem includes many accelerators for data processing like multiplier and multiplier-accumulator. It also has peripherals like UART, MII/RGMII, MDIO, etc. Every ICSSG core includes two 32-bit load/store RISC CPU cores called PRUs. The above features allow it to be used for implementing custom firmware based peripherals like ethernet. This series adds the YAML documentation and the driver with basic EMAC support for TI AM654 Silicon Rev 2 SoC with the PRU_ICSSG Sub-system. running dual-EMAC firmware. This currently supports basic EMAC with 1Gbps and 100Mbps link. 10M and half-duplex modes are not yet supported because they require the support of an IEP, which will be added later. Advanced features like switch-dev and timestamping will be added later. Puranjay Mohan (1): dt-bindings: net: Add ICSSG Ethernet Driver bindings Roger Quadros (1): net: ti: icssg-prueth: Add ICSSG ethernet driver .../bindings/net/ti,icssg-prueth.yaml | 172 ++ .../bindings/remoteproc/ti,pru-consumer.yaml | 66 + drivers/net/ethernet/ti/Kconfig | 15 + drivers/net/ethernet/ti/Makefile | 3 + drivers/net/ethernet/ti/icssg_classifier.c | 375 ++++ drivers/net/ethernet/ti/icssg_config.c | 443 ++++ drivers/net/ethernet/ti/icssg_config.h | 200 ++ drivers/net/ethernet/ti/icssg_ethtool.c | 301 +++ drivers/net/ethernet/ti/icssg_mii_cfg.c | 104 + drivers/net/ethernet/ti/icssg_mii_rt.h | 151 ++ drivers/net/ethernet/ti/icssg_prueth.c | 1891 +++++++++++++++++ drivers/net/ethernet/ti/icssg_prueth.h | 247 +++ drivers/net/ethernet/ti/icssg_switch_map.h | 183 ++ drivers/remoteproc/pru_rproc.c | 234 +- drivers/soc/ti/pruss.c | 257 ++- include/linux/pruss.h | 300 +++ include/linux/pruss_driver.h | 72 +- 17 files changed, 4985 insertions(+), 29 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml create mode 100644 Documentation/devicetree/bindings/remoteproc/ti,pru-consumer.yaml create mode 100644 drivers/net/ethernet/ti/icssg_classifier.c create mode 100644 drivers/net/ethernet/ti/icssg_config.c create mode 100644 drivers/net/ethernet/ti/icssg_config.h create mode 100644 drivers/net/ethernet/ti/icssg_ethtool.c create mode 100644 drivers/net/ethernet/ti/icssg_mii_cfg.c create mode 100644 drivers/net/ethernet/ti/icssg_mii_rt.h create mode 100644 drivers/net/ethernet/ti/icssg_prueth.c create mode 100644 drivers/net/ethernet/ti/icssg_prueth.h create mode 100644 drivers/net/ethernet/ti/icssg_switch_map.h create mode 100644 include/linux/pruss.h