| 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: <5757F504.6050808@suse.com>
Date: Wed, 8 Jun 2016 12:35:48 +0200
From: Matthias Brugger <mbrugger@...e.com>
To: Iyappan Subramanian <isubramanian@....com>, davem@...emloft.net,
netdev@...r.kernel.org, devicetree@...r.kernel.org
Cc: linux-arm-kernel@...ts.infradead.org, patches@....com,
andrew@...n.ch
Subject: Re: [PATCH v3 2/5] drivers: net: xgene: Backward compatibility with
older firmware
On 06/06/16 20:15, Iyappan Subramanian wrote:
> This patch looks for CONFIG_MDIO_XGENE and based on phy-handle DT/ACPI
> fields, sets the mdio_driver flag. The rest of the driver uses the
I'm a bit confused, you introduced mdio_driver flag already in patch 1.
> this flag for any MDIO management, in the case of backward compatibility.
> Also, some code clean up done around mdio configuration/remove.
IMHO code clean up should be part of a different patch.
>
> Signed-off-by: Iyappan Subramanian <isubramanian@....com>
> Tested-by: Fushen Chen <fchen@....com>
> Tested-by: Toan Le <toanle@....com>
> Tested-by: Matthias Brugger <mbrugger@...e.com>
> ---
> drivers/net/ethernet/apm/xgene/xgene_enet_hw.c | 60 +++-----
> drivers/net/ethernet/apm/xgene/xgene_enet_main.c | 165 +++++++++++++++-------
> drivers/net/ethernet/apm/xgene/xgene_enet_main.h | 2 +
> drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c | 13 +-
> 4 files changed, 148 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
> index 5d6d14b..38d6ee4 100644
> --- a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
> +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
> @@ -476,9 +476,13 @@ static void xgene_gmac_reset(struct xgene_enet_pdata *pdata)
> static void xgene_enet_configure_clock(struct xgene_enet_pdata *pdata)
> {
> struct device *dev = &pdata->pdev->dev;
> + struct clk *parent;
>
> if (dev->of_node) {
> - struct clk *parent = clk_get_parent(pdata->clk);
> + if (IS_ERR(pdata->clk))
> + return;
> +
> + parent = clk_get_parent(pdata->clk);
>
> switch (pdata->phy_speed) {
> case SPEED_10:
> @@ -572,6 +576,9 @@ static void xgene_gmac_init(struct xgene_enet_pdata *pdata)
> {
> u32 value;
>
> + if (!pdata->mdio_driver)
> + xgene_gmac_reset(pdata);
> +
> xgene_gmac_set_speed(pdata);
> xgene_gmac_set_mac_addr(pdata);
>
> @@ -677,7 +684,14 @@ static int xgene_enet_reset(struct xgene_enet_pdata *pdata)
> if (!xgene_ring_mgr_init(pdata))
> return -ENODEV;
>
> - xgene_enet_ecc_init(pdata);
> + if (!pdata->mdio_driver) {
> + if (!IS_ERR(pdata->clk)) {
> + clk_prepare_enable(pdata->clk);
> + clk_disable_unprepare(pdata->clk);
> + clk_prepare_enable(pdata->clk);
> + xgene_enet_ecc_init(pdata);
> + }
> + }
In the first patch you add xgene_enet_ecc_init and delete the clock
handling. Here you do it the other way round. And in patch 5 you put
both after calling xgene_enet_config_ring_if_assoc.
Are these changes needed like this in every patch?
> xgene_enet_config_ring_if_assoc(pdata);
>
> return 0;
> @@ -800,27 +814,9 @@ static int xgene_mdiobus_register(struct xgene_enet_pdata *pdata,
> struct mii_bus *mdio)
> {
> struct device *dev = &pdata->pdev->dev;
> - struct device_node *mdio_np = NULL;
> - struct device_node *child_np;
> - u32 phyid;
>
> if (dev->of_node) {
> - for_each_child_of_node(dev->of_node, child_np) {
> - if (of_device_is_compatible(child_np,
> - "apm,xgene-mdio")) {
> - mdio_np = child_np;
> - break;
> - }
> - }
> -
> - if (!mdio_np) {
> - mdiobus_free(mdio);
> - return 0;
> - }
> -
> - pdata->mdio_driver = false;
> -
> - return of_mdiobus_register(mdio, mdio_np);
> + return of_mdiobus_register(mdio, pdata->mdio_np);
> } else {
> #ifdef CONFIG_ACPI
> struct phy_device *phy;
> @@ -839,13 +835,7 @@ static int xgene_mdiobus_register(struct xgene_enet_pdata *pdata,
> if (ret)
> return ret;
>
> - ret = device_property_read_u32(dev, "phy-channel", &phyid);
> - if (ret)
> - ret = device_property_read_u32(dev, "phy-addr", &phyid);
> - if (ret)
> - return -EINVAL;
> -
> - phy = get_phy_device(mdio, phyid, false);
> + phy = get_phy_device(mdio, pdata->phy_id, false);
> if (IS_ERR(phy))
> return -EIO;
>
> @@ -858,6 +848,8 @@ static int xgene_mdiobus_register(struct xgene_enet_pdata *pdata,
> return ret;
> #endif
> }
> +
> + return 0;
> }
>
> int xgene_enet_mdio_config(struct xgene_enet_pdata *pdata)
> @@ -885,10 +877,6 @@ int xgene_enet_mdio_config(struct xgene_enet_pdata *pdata)
> if (mdio_bus->state == MDIOBUS_REGISTERED)
> mdiobus_unregister(pdata->mdio_bus);
> mdiobus_free(mdio_bus);
> - if (pdata->mdio_driver) {
> - ret = xgene_enet_phy_connect(ndev);
> - return 0;
> - }
> return ret;
> }
> pdata->mdio_bus = mdio_bus;
> @@ -911,11 +899,9 @@ void xgene_enet_mdio_remove(struct xgene_enet_pdata *pdata)
> if (pdata->phy_dev)
> phy_disconnect(pdata->phy_dev);
>
> - if (!pdata->mdio_driver) {
> - mdiobus_unregister(pdata->mdio_bus);
> - mdiobus_free(pdata->mdio_bus);
> - pdata->mdio_bus = NULL;
> - }
> + mdiobus_unregister(pdata->mdio_bus);
> + mdiobus_free(pdata->mdio_bus);
> + pdata->mdio_bus = NULL;
> }
>
> const struct xgene_mac_ops xgene_gmac_ops = {
> diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
> index d451e5d..c31ea56 100644
> --- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
> +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
> @@ -1182,31 +1182,27 @@ static const struct net_device_ops xgene_ndev_ops = {
>
> #ifdef CONFIG_ACPI
> static void xgene_get_port_id_acpi(struct device *dev,
> - struct xgene_enet_pdata *pdata)
> + struct xgene_enet_pdata *pdata)
A space slipped in which we don't need.
> {
> acpi_status status;
> u64 temp;
>
> status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_SUN", NULL, &temp);
> - if (ACPI_FAILURE(status)) {
> + if (ACPI_FAILURE(status))
> pdata->port_id = 0;
> - } else {
> + else
> pdata->port_id = temp;
> - }
> -
> - return;
> }
> #endif
>
> -static void xgene_get_port_id_dt(struct device *dev, struct xgene_enet_pdata *pdata)
> +static void xgene_get_port_id_dt(struct device *dev,
> + struct xgene_enet_pdata *pdata)
> {
> u32 id = 0;
>
> of_property_read_u32(dev->of_node, "port-id", &id);
>
> pdata->port_id = id & BIT(0);
> -
> - return;
> }
>
> static int xgene_get_tx_delay(struct xgene_enet_pdata *pdata)
> @@ -1284,6 +1280,86 @@ static int xgene_enet_get_irqs(struct xgene_enet_pdata *pdata)
> return 0;
> }
>
> +static int xgene_enet_check_phy_handle(struct xgene_enet_pdata *pdata)
> +{
> + struct device *dev = &pdata->pdev->dev;
> + int ret, phy_id, phy_mode = pdata->phy_mode;
> + struct device_node *mdio_np = NULL;
> + const char *ph;
> +#ifdef CONFIG_ACPI
> + struct acpi_reference_args args;
> + struct fwnode_handle *fw_node;
> +#endif
> +
> + if (!IS_ENABLED(CONFIG_MDIO_XGENE))
> + return 0;
Kconfig option is introduced in patch 3. So I suppose we should add at
least this lines to this patch. Or at least we should change the order
of the patches, to not introduce a check for a Kconfig option before it
is even present in the kernel. What do you think?
> +
> + if (dev->of_node) {
> + ret = device_property_read_string(dev, "phy-handle", &ph);
> +
> + if (phy_mode == PHY_INTERFACE_MODE_RGMII) {
> + if (ret) {
> + dev_err(dev, "Unable to get phy-handle\n");
> + return ret;
> + }
> +
> + mdio_np = of_find_compatible_node(dev->of_node, NULL,
> + "apm,xgene-mdio");
> + if (!mdio_np)
> + pdata->mdio_driver = true;
> + else
> + pdata->mdio_np = mdio_np;
> + } else {
> + if (!ret)
> + pdata->mdio_driver = true;
> + }
> + } else {
> +#ifdef CONFIG_ACPI
> + fw_node = acpi_fwnode_handle(ACPI_COMPANION(dev));
> + ret = acpi_node_get_property_reference(fw_node, "mboxes", 0,
> + &args);
> + if (!ACPI_FAILURE(ret)) {
> + pdata->mdio_driver = true;
> + pdata->phy_dev = args.adev->driver_data;
> + } else {
> + ret = device_property_read_u32(dev, "phy-channel",
> + &phy_id);
> + if (ret) {
> + ret = device_property_read_u32(dev, "phy-addr",
> + &phy_id);
> + }
> +
> + if (!ret)
> + pdata->phy_id = phy_id;
> + }
> +#endif
> + }
> +
> + return 0;
> +}
> +
> +static int xgene_enet_get_clk(struct xgene_enet_pdata *pdata)
> +{
> + struct device *dev = &pdata->pdev->dev;
> +
> + if (!dev->of_node)
> + return 0;
> +
> + pdata->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(pdata->clk)) {
> + if (pdata->phy_mode == PHY_INTERFACE_MODE_RGMII) {
> + dev_err(dev, "Unable to get clock\n");
> + return -ENODEV;
> + } else {
> + /* Firmware may have set up the clock already. */
> + dev_info(dev, "clocks have been setup already\n");
AFAIK clk_get does not check if the bootloader has set up the clock. It
just gives you a reference to a clock that was defined in the clock
driver of your SOC. So for me this comment and dev_info make no sense.
Regards,
Matthias
> + pdata->clk = NULL;
> + }
> + }
> +
> + return 0;
> +}
> +
> static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata)
> {
> struct platform_device *pdev;
> @@ -1292,7 +1368,6 @@ static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata)
> struct resource *res;
> void __iomem *base_addr;
> u32 offset;
> - const char *ph;
> int ret = 0;
>
> pdev = pdata->pdev;
> @@ -1370,15 +1445,13 @@ static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata)
> if (ret)
> return ret;
>
> - ret = device_property_read_string(dev, "phy-handle", &ph);
> - if (!ret)
> - pdata->mdio_driver = true;
> + ret = xgene_enet_check_phy_handle(pdata);
> + if (ret)
> + return ret;
>
> - pdata->clk = devm_clk_get(&pdev->dev, NULL);
> - if (IS_ERR(pdata->clk)) {
> - /* Firmware may have set up the clock already. */
> - dev_info(dev, "clocks have been setup already\n");
> - }
> + ret = xgene_enet_get_clk(pdata);
> + if (ret)
> + return ret;
>
> if ((pdata->phy_mode != PHY_INTERFACE_MODE_XGMII) &&
> (pdata->enet_id == XGENE_ENET1))
> @@ -1434,8 +1507,6 @@ static int xgene_enet_init_hw(struct xgene_enet_pdata *pdata)
> }
> }
>
> - dst_ring_num = xgene_enet_dst_ring_num(pdata->rx_ring[0]);
> - buf_pool = pdata->rx_ring[0]->buf_pool;
> if (pdata->phy_mode == PHY_INTERFACE_MODE_XGMII) {
> /* Initialize and Enable PreClassifier Tree */
> enet_cle->max_nodes = 512;
> @@ -1451,9 +1522,12 @@ static int xgene_enet_init_hw(struct xgene_enet_pdata *pdata)
> return ret;
> }
> } else {
> + dst_ring_num = xgene_enet_dst_ring_num(pdata->rx_ring[0]);
> + buf_pool = pdata->rx_ring[0]->buf_pool;
> pdata->port_ops->cle_bypass(pdata, dst_ring_num, buf_pool->id);
> }
>
> + pdata->phy_speed = SPEED_UNKNOWN;
> pdata->mac_ops->init(pdata);
>
> return ret;
> @@ -1563,22 +1637,6 @@ static void xgene_enet_napi_add(struct xgene_enet_pdata *pdata)
> }
> }
>
> -static void xgene_enet_napi_del(struct xgene_enet_pdata *pdata)
> -{
> - struct napi_struct *napi;
> - int i;
> -
> - for (i = 0; i < pdata->rxq_cnt; i++) {
> - napi = &pdata->rx_ring[i]->napi;
> - netif_napi_del(napi);
> - }
> -
> - for (i = 0; i < pdata->cq_cnt; i++) {
> - napi = &pdata->tx_ring[i]->cp_ring->napi;
> - netif_napi_del(napi);
> - }
> -}
> -
> static int xgene_enet_probe(struct platform_device *pdev)
> {
> struct net_device *ndev;
> @@ -1609,8 +1667,7 @@ static int xgene_enet_probe(struct platform_device *pdev)
> of_id = of_match_device(xgene_enet_of_match, &pdev->dev);
> if (of_id) {
> pdata->enet_id = (enum xgene_enet_id)of_id->data;
> - }
> - else {
> + } else {
> #ifdef CONFIG_ACPI
> const struct acpi_device_id *acpi_id;
> enum xgene_enet_id enet_id;
> @@ -1622,6 +1679,7 @@ static int xgene_enet_probe(struct platform_device *pdev)
> }
> #endif
> }
> +
> if (!pdata->enet_id) {
> free_netdev(ndev);
> return -ENODEV;
> @@ -1656,23 +1714,25 @@ static int xgene_enet_probe(struct platform_device *pdev)
> goto err_netdev;
>
> link_state = pdata->mac_ops->link_state;
> - if (pdata->phy_mode == PHY_INTERFACE_MODE_RGMII) {
> - ret = xgene_enet_mdio_config(pdata);
> - if (ret)
> - goto err_netdev;
> - } else if (pdata->phy_mode == PHY_INTERFACE_MODE_SGMII) {
> + ret = 0;
> + if (pdata->phy_mode == PHY_INTERFACE_MODE_XGMII) {
> + INIT_DELAYED_WORK(&pdata->link_work, link_state);
> + } else {
> if (pdata->mdio_driver)
> ret = xgene_enet_phy_connect(ndev);
> + else if (pdata->phy_mode == PHY_INTERFACE_MODE_RGMII)
> + ret = xgene_enet_mdio_config(pdata);
> else
> INIT_DELAYED_WORK(&pdata->link_work, link_state);
> - } else {
> - INIT_DELAYED_WORK(&pdata->link_work, link_state);
> }
> + if (ret)
> + goto err;
>
> xgene_enet_napi_add(pdata);
> return 0;
> err_netdev:
> - unregister_netdev(ndev);
> + if (ndev->reg_state == NETREG_REGISTERED)
> + unregister_netdev(ndev);
> err:
> free_netdev(ndev);
> return ret;
> @@ -1691,11 +1751,16 @@ static int xgene_enet_remove(struct platform_device *pdev)
> mac_ops->rx_disable(pdata);
> mac_ops->tx_disable(pdata);
>
> - xgene_enet_napi_del(pdata);
> - if (pdata->phy_mode == PHY_INTERFACE_MODE_RGMII)
> - xgene_enet_mdio_remove(pdata);
> - else if (pdata->phy_mode == PHY_INTERFACE_MODE_SGMII)
> + rtnl_lock();
> + if (netif_running(ndev))
> + dev_close(ndev);
> + rtnl_unlock();
> +
> + if (pdata->mdio_driver)
> xgene_enet_phy_disconnect(pdata);
> + else if (pdata->phy_mode == PHY_INTERFACE_MODE_RGMII)
> + xgene_enet_mdio_remove(pdata);
> +
> unregister_netdev(ndev);
> xgene_enet_delete_desc_rings(pdata);
> pdata->port_ops->shutdown(pdata);
> diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.h b/drivers/net/ethernet/apm/xgene/xgene_enet_main.h
> index 0fe1a96..c2804c2 100644
> --- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.h
> +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.h
> @@ -212,6 +212,8 @@ struct xgene_enet_pdata {
> u32 mss;
> u8 tx_delay;
> u8 rx_delay;
> + struct device_node *mdio_np;
> + int phy_id;
> bool mdio_driver;
> };
>
> diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c b/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c
> index a7a6c05..ae93dc2 100644
> --- a/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c
> +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c
> @@ -257,9 +257,7 @@ static void xgene_sgmac_set_speed(struct xgene_enet_pdata *p)
> u32 mc2, value;
>
> if (p->phy_speed != SPEED_UNKNOWN) {
> - value = xgene_mii_phy_read(p, INT_PHY_ADDR,
> - SGMII_BASE_PAGE_ABILITY_ADDR >> 2);
> - if (!(value & LINK_UP)) {
> + if (!xgene_enet_link_status(p)) {
> xgene_mii_phy_write(p, INT_PHY_ADDR,
> SGMII_TBI_CONTROL_ADDR >> 2,
> 0x8000);
> @@ -325,7 +323,8 @@ static void xgene_sgmac_init(struct xgene_enet_pdata *p)
> u32 enet_spare_cfg_reg, rsif_config_reg;
> u32 cfg_bypass_reg, rx_dv_gate_reg;
>
> - xgene_sgmac_reset(p);
> + if (!(p->enet_id == XGENE_ENET2 && p->mdio_driver))
> + xgene_sgmac_reset(p);
>
> /* Enable auto-negotiation */
> xgene_mii_phy_write(p, INT_PHY_ADDR, SGMII_TBI_CONTROL_ADDR >> 2,
> @@ -416,6 +415,8 @@ static void xgene_sgmac_tx_disable(struct xgene_enet_pdata *p)
>
> static int xgene_enet_reset(struct xgene_enet_pdata *p)
> {
> + int ret;
> +
> if (!xgene_ring_mgr_init(p))
> return -ENODEV;
>
> @@ -428,7 +429,9 @@ static int xgene_enet_reset(struct xgene_enet_pdata *p)
> clk_prepare_enable(p->clk);
> }
>
> - xgene_enet_ecc_init(p);
> + ret = xgene_enet_ecc_init(p);
> + if (ret)
> + return ret;
> xgene_enet_config_ring_if_assoc(p);
>
> return 0;
>
Powered by blists - more mailing lists