| Message ID | 1507854928-4357-1-git-send-email-hayashi.kunihiko@socionext.com |
|---|---|
| Headers | show |
| Series | add UniPhier AVE ethernet support | expand |
2017-10-13 9:35 GMT+09:00 Kunihiko Hayashi <hayashi.kunihiko@socionext.com>: > +static int ave_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + u32 ave_id; > + struct ave_private *priv; > + const struct ave_soc_data *data; > + phy_interface_t phy_mode; > + struct net_device *ndev; > + struct resource *res; > + void __iomem *base; > + int irq, ret = 0; > + char buf[ETHTOOL_FWVERS_LEN]; > + const void *mac_addr; > + > + data = of_device_get_match_data(dev); > + if (WARN_ON(!data)) > + return -EINVAL; > + > + phy_mode = of_get_phy_mode(np); > + if (phy_mode < 0) { > + dev_err(dev, "phy-mode not found\n"); > + return -EINVAL; > + } > + if ((!phy_interface_mode_is_rgmii(phy_mode)) && > + phy_mode != PHY_INTERFACE_MODE_RMII && > + phy_mode != PHY_INTERFACE_MODE_MII) { > + dev_err(dev, "phy-mode is invalid\n"); > + return -EINVAL; > + } > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(dev, "IRQ not found\n"); > + return irq; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + /* alloc netdevice */ > + ndev = alloc_etherdev(sizeof(struct ave_private)); > + if (!ndev) { > + dev_err(dev, "can't allocate ethernet device\n"); > + return -ENOMEM; > + } > + > + ndev->netdev_ops = &ave_netdev_ops; > + ndev->ethtool_ops = &ave_ethtool_ops; > + SET_NETDEV_DEV(ndev, dev); > + > + /* support hardware checksum */ > + ndev->features |= (NETIF_F_IP_CSUM | NETIF_F_RXCSUM); > + ndev->hw_features |= (NETIF_F_IP_CSUM | NETIF_F_RXCSUM); > + > + ndev->max_mtu = AVE_MAX_ETHFRAME - (ETH_HLEN + ETH_FCS_LEN); > + > + /* get mac address */ > + mac_addr = of_get_mac_address(np); > + if (mac_addr) > + ether_addr_copy(ndev->dev_addr, mac_addr); > + > + /* if the mac address is invalid, use random mac address */ > + if (!is_valid_ether_addr(ndev->dev_addr)) { > + eth_hw_addr_random(ndev); > + dev_warn(dev, "Using random MAC address: %pM\n", > + ndev->dev_addr); > + } > + > + priv = netdev_priv(ndev); > + priv->base = base; > + priv->irq = irq; > + priv->ndev = ndev; > + priv->msg_enable = netif_msg_init(-1, AVE_DEFAULT_MSG_ENABLE); > + priv->phy_mode = phy_mode; > + priv->data = data; > + > + if (IS_DESC_64BIT(priv)) { > + priv->desc_size = AVE_DESC_SIZE_64; > + priv->tx.daddr = AVE_TXDM_64; > + priv->rx.daddr = AVE_RXDM_64; > + } else { > + priv->desc_size = AVE_DESC_SIZE_32; > + priv->tx.daddr = AVE_TXDM_32; > + priv->rx.daddr = AVE_RXDM_32; > + } > + priv->tx.ndesc = AVE_NR_TXDESC; > + priv->rx.ndesc = AVE_NR_RXDESC; > + > + u64_stats_init(&priv->stats_rx.syncp); > + u64_stats_init(&priv->stats_tx.syncp); > + > + /* get clock */ Please remove this super-obvious comment. > + priv->clk = clk_get(dev, NULL); Missing clk_put() in the failure path. Why don't you use devm? > + if (IS_ERR(priv->clk)) > + priv->clk = NULL; So, clk is optional, but you need to check EPROBE_DEFER. > + /* get reset */ Remove. > + priv->rst = reset_control_get(dev, NULL); > + if (IS_ERR(priv->rst)) > + priv->rst = NULL; reset_control_get() is deprecated. Do not use it in a new driver. Again, missing reset_control_put(). devm? The reset seems optional (again, ignoring EPROBE_DEFER) but you did not use reset_control_get_optional, why? >From your code, this reset is used as shared. priv->rst = devm_reset_control_get_optional_shared(dev, NULL); if (IS_ERR(priv->rst)) return PTR_ERR(priv->rst);
On Mon, 16 Oct 2017 00:08:21 +0900 <yamada.masahiro@socionext.com> wrote: > 2017-10-13 9:35 GMT+09:00 Kunihiko Hayashi <hayashi.kunihiko@socionext.com>: > > +static int ave_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct device_node *np = dev->of_node; > > + u32 ave_id; > > + struct ave_private *priv; > > + const struct ave_soc_data *data; > > + phy_interface_t phy_mode; > > + struct net_device *ndev; > > + struct resource *res; > > + void __iomem *base; > > + int irq, ret = 0; > > + char buf[ETHTOOL_FWVERS_LEN]; > > + const void *mac_addr; > > + > > + data = of_device_get_match_data(dev); > > + if (WARN_ON(!data)) > > + return -EINVAL; > > + > > + phy_mode = of_get_phy_mode(np); > > + if (phy_mode < 0) { > > + dev_err(dev, "phy-mode not found\n"); > > + return -EINVAL; > > + } > > + if ((!phy_interface_mode_is_rgmii(phy_mode)) && > > + phy_mode != PHY_INTERFACE_MODE_RMII && > > + phy_mode != PHY_INTERFACE_MODE_MII) { > > + dev_err(dev, "phy-mode is invalid\n"); > > + return -EINVAL; > > + } > > + > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) { > > + dev_err(dev, "IRQ not found\n"); > > + return irq; > > + } > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + base = devm_ioremap_resource(dev, res); > > + if (IS_ERR(base)) > > + return PTR_ERR(base); > > + > > + /* alloc netdevice */ > > + ndev = alloc_etherdev(sizeof(struct ave_private)); > > + if (!ndev) { > > + dev_err(dev, "can't allocate ethernet device\n"); > > + return -ENOMEM; > > + } > > + > > + ndev->netdev_ops = &ave_netdev_ops; > > + ndev->ethtool_ops = &ave_ethtool_ops; > > + SET_NETDEV_DEV(ndev, dev); > > + > > + /* support hardware checksum */ > > + ndev->features |= (NETIF_F_IP_CSUM | NETIF_F_RXCSUM); > > + ndev->hw_features |= (NETIF_F_IP_CSUM | NETIF_F_RXCSUM); > > + > > + ndev->max_mtu = AVE_MAX_ETHFRAME - (ETH_HLEN + ETH_FCS_LEN); > > + > > + /* get mac address */ > > + mac_addr = of_get_mac_address(np); > > + if (mac_addr) > > + ether_addr_copy(ndev->dev_addr, mac_addr); > > + > > + /* if the mac address is invalid, use random mac address */ > > + if (!is_valid_ether_addr(ndev->dev_addr)) { > > + eth_hw_addr_random(ndev); > > + dev_warn(dev, "Using random MAC address: %pM\n", > > + ndev->dev_addr); > > + } > > + > > + priv = netdev_priv(ndev); > > + priv->base = base; > > + priv->irq = irq; > > + priv->ndev = ndev; > > + priv->msg_enable = netif_msg_init(-1, AVE_DEFAULT_MSG_ENABLE); > > + priv->phy_mode = phy_mode; > > + priv->data = data; > > + > > + if (IS_DESC_64BIT(priv)) { > > + priv->desc_size = AVE_DESC_SIZE_64; > > + priv->tx.daddr = AVE_TXDM_64; > > + priv->rx.daddr = AVE_RXDM_64; > > + } else { > > + priv->desc_size = AVE_DESC_SIZE_32; > > + priv->tx.daddr = AVE_TXDM_32; > > + priv->rx.daddr = AVE_RXDM_32; > > + } > > + priv->tx.ndesc = AVE_NR_TXDESC; > > + priv->rx.ndesc = AVE_NR_RXDESC; > > + > > + u64_stats_init(&priv->stats_rx.syncp); > > + u64_stats_init(&priv->stats_tx.syncp); > > + > > + /* get clock */ > > Please remove this super-obvious comment. I'll check comments out and remove them unnecessary. > > + priv->clk = clk_get(dev, NULL); > > Missing clk_put() in the failure path. > > Why don't you use devm? > > > > > + if (IS_ERR(priv->clk)) > > + priv->clk = NULL; > > So, clk is optional, but > you need to check EPROBE_DEFER. > > > > > > + /* get reset */ > > Remove. > > > > + priv->rst = reset_control_get(dev, NULL); > > + if (IS_ERR(priv->rst)) > > + priv->rst = NULL; > > > reset_control_get() is deprecated. Do not use it in a new driver. > > Again, missing reset_control_put(). devm? > > > The reset seems optional (again, ignoring EPROBE_DEFER) > but you did not use reset_control_get_optional, why? > > From your code, this reset is used as shared. > > > priv->rst = devm_reset_control_get_optional_shared(dev, NULL); > if (IS_ERR(priv->rst)) > return PTR_ERR(priv->rst); The clk and reset are optional in the driver. Referring to your suggested method, I'll fix the part of clk and reset. > > > -- > Best Regards > Masahiro Yamada --- Best Regards, Kunihiko Hayashi -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2017-10-18 19:23 GMT+09:00 Kunihiko Hayashi <hayashi.kunihiko@socionext.com>: > On Mon, 16 Oct 2017 00:08:21 +0900 <yamada.masahiro@socionext.com> wrote: >> priv->rst = devm_reset_control_get_optional_shared(dev, NULL); >> if (IS_ERR(priv->rst)) >> return PTR_ERR(priv->rst); > > The clk and reset are optional in the driver. > Referring to your suggested method, I'll fix the part of clk and reset. > Why is clk optional?
On Thu, 19 Oct 2017 09:29:03 +0900 <yamada.masahiro@socionext.com> wrote: > 2017-10-18 19:23 GMT+09:00 Kunihiko Hayashi <hayashi.kunihiko@socionext.com>: > > On Mon, 16 Oct 2017 00:08:21 +0900 <yamada.masahiro@socionext.com> wrote: > > >> priv->rst = devm_reset_control_get_optional_shared(dev, NULL); > >> if (IS_ERR(priv->rst)) > >> return PTR_ERR(priv->rst); > > > > The clk and reset are optional in the driver. > > Referring to your suggested method, I'll fix the part of clk and reset. > > > > > Why is clk optional? Indeed. My check point was wrong. This clk property should be treated as "required". In v2, the clock enabling code moved to ndo_init(). Although probing the driver succeeds without clk, enabling the driver fails clearly. --- Best Regards, Kunihiko Hayashi -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
This series adds support for Socionext AVE ethernet controller implemented on UniPhier SoCs. This driver supports RGMII/RMII modes. v1: http://www.spinics.net/lists/netdev/msg454292.html The PHY patch included in v1 has already separated in: http://www.spinics.net/lists/netdev/msg454595.html Changes since v1: - add/remove devicetree properties and sub-node - remove "internal-phy-interrupt" and "desc-bits" property - add SoC data structures based on compatible strings - add node operation to apply "mdio" sub-node - add support for features - add support for {get,set}_pauseparam and pause frame operations - add support for ndo_get_stats64 instead of ndo_get_stats - replace with desiable functions - replace check for valid phy_mode with phy_interface{_mode}_is_rgmii() - replace phy attach message with phy_attached_info() - replace 32bit operation with {upper,lower}_32_bits() on ave_wdesc_addr() - replace nway_reset and get_link with generic functions - move operations to proper functions - move phy_start_aneg() to ndo_open, and remove unnecessary PHY interrupt operations See http://www.spinics.net/lists/netdev/msg454590.html - move irq initialization and descriptor memory allocation to ndo_open - move initialization of reset and clock and mdiobus to ndo_init - fix skbuffer operations - fix skb alignment operations and add Rx buffer adjustment for descriptor See http://www.spinics.net/lists/netdev/msg456014.html - add error returns when dma_map_single() failed - clean up code structures - clean up wait-loop and wake-queue conditions - add ave_wdesc_addr() and offset definitions - add ave_macaddr_init() to clean up mac-address operation - fix checking whether Tx entry is not enough - fix supported features of phydev - add necessary free/disable operations - add phydev check on ave_{get,set}_wol() - remove netif_carrier functions, phydev initializer, and Tx budget check - change obsolate codes - replace ndev->{base_addr,irq} with the members of ave_private - rename goto labels and mask definitions, and remove unused codes Kunihiko Hayashi (2): dt-bindings: net: add DT bindings for Socionext UniPhier AVE net: ethernet: socionext: add AVE ethernet driver .../bindings/net/socionext,uniphier-ave4.txt | 53 + drivers/net/ethernet/Kconfig | 1 + drivers/net/ethernet/Makefile | 1 + drivers/net/ethernet/socionext/Kconfig | 22 + drivers/net/ethernet/socionext/Makefile | 4 + drivers/net/ethernet/socionext/sni_ave.c | 1773 ++++++++++++++++++++ 6 files changed, 1854 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt create mode 100644 drivers/net/ethernet/socionext/Kconfig create mode 100644 drivers/net/ethernet/socionext/Makefile create mode 100644 drivers/net/ethernet/socionext/sni_ave.c