| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <543b8c11-db95-29d1-29bc-ae5cbd99b2e2@ti.com>
Date: Tue, 12 Apr 2022 15:12:37 +0530
From: Puranjay Mohan <p-mohan@...com>
To: Andrew Lunn <andrew@...n.ch>
CC: <linux-kernel@...r.kernel.org>, <bjorn.andersson@...aro.org>,
<mathieu.poirier@...aro.org>, <krzysztof.kozlowski+dt@...aro.org>,
<linux-remoteproc@...r.kernel.org>, <devicetree@...r.kernel.org>,
<nm@...com>, <ssantosh@...nel.org>, <s-anna@...com>,
<linux-arm-kernel@...ts.infradead.org>, <davem@...emloft.net>,
<kuba@...nel.org>, <netdev@...r.kernel.org>, <vigneshr@...com>,
<kishon@...com>, Grygorii Strashko <grygorii.strashko@...com>,
Roger Quadros <rogerq@...nel.org>
Subject: Re: [RFC 13/13] net: ti: icssg-prueth: Add ICSSG ethernet driver
+ 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
Powered by blists - more mailing lists