| Message ID | 20170610034630.493852-7-salil.mehta@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | Hisilicon Network Subsystem 3 Ethernet Driver | expand |
Le 06/09/17 à 20:46, Salil Mehta a écrit : > This patch adds the support of MDIO bus interface for HNS3 driver. > Code provides various interfaces to start and stop the PHY layer > and to read and write the MDIO bus or PHY. > > Signed-off-by: Daode Huang <huangdaode@hisilicon.com> > Signed-off-by: lipeng <lipeng321@huawei.com> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > Signed-off-by: Yisen Zhuang <yisen.zhuang@huawei.com> > --- > .../ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c | 310 +++++++++++++++++++++ > 1 file changed, 310 insertions(+) > create mode 100644 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c > new file mode 100644 > index 0000000..c6812d2 > --- /dev/null > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c > @@ -0,0 +1,310 @@ > +/* > + * Copyright (c) 2016~2017 Hisilicon Limited. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#include <linux/etherdevice.h> > +#include <linux/kernel.h> > + > +#include "hclge_cmd.h" > +#include "hclge_main.h" > + > +enum hclge_mdio_c22_op_seq { > + HCLGE_MDIO_C22_WRITE = 1, > + HCLGE_MDIO_C22_READ = 2 > +}; > + > +enum hclge_mdio_c45_op_seq { > + HCLGE_MDIO_C45_WRITE_ADDR = 0, > + HCLGE_MDIO_C45_WRITE_DATA, > + HCLGE_MDIO_C45_READ_INCREMENT, > + HCLGE_MDIO_C45_READ > +}; > + > +#define HCLGE_MDIO_CTRL_START_BIT BIT(0) > +#define HCLGE_MDIO_CTRL_ST_MSK GENMASK(2, 1) > +#define HCLGE_MDIO_CTRL_ST_LSH 1 > +#define HCLGE_MDIO_IS_C22(c22) (((c22) << HCLGE_MDIO_CTRL_ST_LSH) & \ > + HCLGE_MDIO_CTRL_ST_MSK) > + > +#define HCLGE_MDIO_CTRL_OP_MSK GENMASK(4, 3) > +#define HCLGE_MDIO_CTRL_OP_LSH 3 > +#define HCLGE_MDIO_CTRL_OP(access) \ > + (((access) << HCLGE_MDIO_CTRL_OP_LSH) & HCLGE_MDIO_CTRL_OP_MSK) > +#define HCLGE_MDIO_CTRL_PRTAD_MSK GENMASK(4, 0) > +#define HCLGE_MDIO_CTRL_DEVAD_MSK GENMASK(4, 0) > + > +#define HCLGE_MDIO_STA_VAL(val) ((val) & BIT(0)) > + > +struct hclge_mdio_cfg_cmd { > + u8 ctrl_bit; > + u8 prtad; /* The external port address */ > + u8 devad; /* The external device address */ > + u8 rsvd; > + __le16 addr_c45;/* Only valid for c45 */ > + __le16 data_wr; > + __le16 data_rd; > + __le16 sta;> +}; > + > +static int hclge_mdio_write(struct mii_bus *bus, int phy_id, int regnum, > + u16 data) > +{ > + struct hclge_dev *hdev = (struct hclge_dev *)bus->priv; > + struct hclge_mdio_cfg_cmd *mdio_cmd; > + enum hclge_cmd_status status; > + struct hclge_desc desc; > + u8 is_c45, devad; > + u16 reg; > + > + if (!bus) > + return -EINVAL; > + > + is_c45 = !!(regnum & MII_ADDR_C45); > + devad = ((regnum >> 16) & 0x1f); > + reg = (u16)(regnum & 0xffff); > + > + hclge_cmd_setup_basic_desc(&desc, HCLGE_OPC_MDIO_CONFIG, false); > + > + mdio_cmd = (struct hclge_mdio_cfg_cmd *)desc.data; > + > + if (!is_c45) { It would be more readable with positive logic: if (is_c45) { } else { } > + /* C22 write reg and data */ > + mdio_cmd->ctrl_bit = HCLGE_MDIO_IS_C22(!is_c45); > + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_OP(HCLGE_MDIO_C22_WRITE); > + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_START_BIT; > + mdio_cmd->data_wr = cpu_to_le16(data); > + mdio_cmd->devad = devad & HCLGE_MDIO_CTRL_DEVAD_MSK; > + mdio_cmd->prtad = phy_id & HCLGE_MDIO_CTRL_PRTAD_MSK; > + } else { > + /* Set phy addr */ > + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_START_BIT; > + mdio_cmd->addr_c45 = cpu_to_le16(reg); > + mdio_cmd->data_wr = cpu_to_le16(data); > + mdio_cmd->devad = devad & HCLGE_MDIO_CTRL_DEVAD_MSK; > + mdio_cmd->prtad = phy_id & HCLGE_MDIO_CTRL_PRTAD_MSK; > + } There is some common initialization that you could probably extracted out of the C22/C45 clause here. > + > + status = hclge_cmd_send(&hdev->hw, &desc, 1); > + if (status) { > + dev_err(&hdev->pdev->dev, > + "mdio write fail when sending cmd, status is %d.\n", > + status); > + return -EIO; > + } > + > + return 0; > +} > + > +static int hclge_mdio_read(struct mii_bus *bus, int phy_id, int regnum) > +{ > + struct hclge_dev *hdev = (struct hclge_dev *)bus->priv; > + struct hclge_mdio_cfg_cmd *mdio_cmd; > + enum hclge_cmd_status status; > + struct hclge_desc desc; > + u8 is_c45, devad; > + u16 reg; > + > + if (!bus) > + return -EINVAL; > + > + is_c45 = !!(regnum & MII_ADDR_C45); > + devad = ((regnum >> 16) & GENMASK(4, 0)); > + reg = (u16)(regnum & GENMASK(15, 0)); > + > + hclge_cmd_setup_basic_desc(&desc, HCLGE_OPC_MDIO_CONFIG, true); > + > + mdio_cmd = (struct hclge_mdio_cfg_cmd *)desc.data; > + > + dev_dbg(&bus->dev, "phy id=%d, is_c45=%d, devad=%d, reg=%#x!\n", > + phy_id, is_c45, devad, reg); > + > + if (!is_c45) { > + /* C22 read reg */ > + mdio_cmd->ctrl_bit = HCLGE_MDIO_IS_C22(!is_c45); > + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_OP(HCLGE_MDIO_C22_READ); > + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_START_BIT; > + mdio_cmd->devad = reg & HCLGE_MDIO_CTRL_DEVAD_MSK; > + mdio_cmd->prtad = phy_id & HCLGE_MDIO_CTRL_PRTAD_MSK; > + } else { > + /* C45 phy addr */ > + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_START_BIT; > + > + mdio_cmd->addr_c45 = cpu_to_le16(reg); > + mdio_cmd->devad = devad & HCLGE_MDIO_CTRL_DEVAD_MSK; > + mdio_cmd->prtad = phy_id & HCLGE_MDIO_CTRL_PRTAD_MSK; > + } Same here, the last two assignments could be common with an appropriate variable assignment before. > + > + /* Read out phy data */ > + status = hclge_cmd_send(&hdev->hw, &desc, 1); > + if (status) { > + dev_err(&hdev->pdev->dev, > + "mdio read fail when get data, status is %d.\n", > + status); > + return -EIO; > + } > + > + if (le16_to_cpu(HCLGE_MDIO_STA_VAL(mdio_cmd->sta))) { > + dev_err(&hdev->pdev->dev, "mdio read data error\n"); > + return -ENOMEM; > + } > + > + return le16_to_cpu(mdio_cmd->data_rd); > +} > + > +int hclge_mac_mdio_config(struct hclge_dev *hdev) > +{ > + struct hclge_mac *mac = &hdev->hw.mac; > + struct mii_bus *mdio_bus; > + struct net_device *ndev = &mac->ndev; > + struct phy_device *phy; > + bool is_c45; > + int ret; > + > + if (hdev->hw.mac.phy_addr >= PHY_MAX_ADDR) > + return 0; > + > + if (hdev->hw.mac.phy_if == PHY_INTERFACE_MODE_NA) > + return 0; > + else if (mac->phy_if == PHY_INTERFACE_MODE_SGMII) > + is_c45 = 0; > + else if (mac->phy_if == PHY_INTERFACE_MODE_XGMII) > + is_c45 = 1; > + else > + return -ENODATA; Can you consider using a switch () case statement here? > + > + SET_NETDEV_DEV(ndev, &hdev->pdev->dev); > + > + mdio_bus = devm_mdiobus_alloc(&hdev->pdev->dev); > + if (!mdio_bus) { > + ret = -ENOMEM; > + goto err_miibus_alloc; > + } > + > + mdio_bus->name = "hisilicon MII bus"; > + mdio_bus->read = hclge_mdio_read; > + mdio_bus->write = hclge_mdio_write; > + snprintf(mdio_bus->id, MII_BUS_ID_SIZE, "%s-%s", "mii", > + dev_name(&hdev->pdev->dev)); > + > + mdio_bus->parent = &hdev->pdev->dev; > + mdio_bus->priv = hdev; > + mdio_bus->phy_mask = ~0; > + ret = mdiobus_register(mdio_bus); > + if (ret) { > + dev_err(mdio_bus->parent, > + "Failed to register MDIO bus ret = %#x\n", ret); > + goto err_mdio_register; > + } > + > + phy = get_phy_device(mdio_bus, mac->phy_addr, is_c45); > + if (!phy || IS_ERR(phy)) { > + dev_err(mdio_bus->parent, "Failed to get phy device\n"); > + ret = -EIO; > + goto err_mdio_register; > + } Hum why do you do this? mdiobus_register() will scan through your bus provided that you set an appropriate phy_mask value (here you tell it not to) and you already provide the PHY address to scan for > + > + phy->irq = mdio_bus->irq[mac->phy_addr]; > + > + /* All data is now stored in the phy struct; > + * register it > + */ > + ret = phy_device_register(phy); > + if (ret) { > + ret = -ENODEV; > + goto err_phy_register; > + } This also looks completely unecessary if you correctly set phy_mask. Can you explain why you did that way? > + > + mac->phy_dev = phy; > + > + return 0; > + > +err_phy_register: > + phy_device_free(phy); > +err_mdio_register: > + mdiobus_unregister(mdio_bus); > + mdiobus_free(mdio_bus); > +err_miibus_alloc: > + return ret; > +} > + > +static void hclge_mac_adjust_link(struct net_device *net_dev) > +{ > + int duplex; > + int speed; > + struct hclge_mac *hw_mac; > + struct hclge_hw *hw; > + struct hclge_dev *hdev; > + > + if (!net_dev) > + return; > + > + hw_mac = container_of(net_dev, struct hclge_mac, ndev); > + hw = container_of(hw_mac, struct hclge_hw, mac); > + hdev = hw->back; > + > + speed = hw_mac->phy_dev->speed; > + duplex = hw_mac->phy_dev->duplex; > + > + /* update antoneg. */ > + hw_mac->autoneg = hw_mac->phy_dev->autoneg; > + > + if ((hw_mac->speed != speed) || (hw_mac->duplex != duplex)) > + (void)hclge_cfg_mac_speed_dup(hdev, speed, !!duplex); > +} > + > +int hclge_mac_start_phy(struct hclge_dev *hdev) > +{ > + struct hclge_mac *mac = &hdev->hw.mac; > + struct phy_device *phy_dev = mac->phy_dev; > + int ret; > + > + if (!phy_dev) > + return 0; > + > + if (mac->phy_if != PHY_INTERFACE_MODE_XGMII) { > + phy_dev->dev_flags = 0; > + > + ret = phy_connect_direct(&mac->ndev, phy_dev, > + hclge_mac_adjust_link, > + mac->phy_if); > + phy_dev->supported = SUPPORTED_10baseT_Half | > + SUPPORTED_10baseT_Full | > + SUPPORTED_100baseT_Half | > + SUPPORTED_100baseT_Full | > + SUPPORTED_Autoneg | > + SUPPORTED_1000baseT_Full; > + > + phy_dev->autoneg = false; > + } else { > + ret = phy_attach_direct(&mac->ndev, phy_dev, 0, mac->phy_if); > + phy_dev->supported = SUPPORTED_10000baseR_FEC | > + SUPPORTED_10000baseKR_Full; > + } Why is there a different PHYLIB call based on XGMII or not? Cannot you always attach to the PHY and then start the PHY state machine instead of doing a connect() or attach() here? > + if (unlikely(ret)) > + return -ENODEV; > + > + phy_start(phy_dev); > + > + return 0; > +} > + > +void hclge_mac_stop_phy(struct hclge_dev *hdev) > +{ > + struct hclge_mac *mac = &hdev->hw.mac; > + struct phy_device *phy_dev = mac->phy_dev; > + > + if (!phy_dev) > + return; > + > + phy_stop(phy_dev); > + > + if (mac->phy_if != PHY_INTERFACE_MODE_XGMII) > + phy_disconnect(phy_dev); > + else > + phy_detach(phy_dev); You cannot detach or disconnect based on the PHY interface, that does not really make sense as both are not symetrical. -- Florian
> > +int hclge_mac_mdio_config(struct hclge_dev *hdev) > > +{ > > + struct hclge_mac *mac = &hdev->hw.mac; > > + struct mii_bus *mdio_bus; > > + struct net_device *ndev = &mac->ndev; > > + struct phy_device *phy; > > + bool is_c45; > > + int ret; > > + > > + if (hdev->hw.mac.phy_addr >= PHY_MAX_ADDR) > > + return 0; > > + > > + if (hdev->hw.mac.phy_if == PHY_INTERFACE_MODE_NA) > > + return 0; > > + else if (mac->phy_if == PHY_INTERFACE_MODE_SGMII) > > + is_c45 = 0; > > + else if (mac->phy_if == PHY_INTERFACE_MODE_XGMII) > > + is_c45 = 1; > > + else > > + return -ENODATA; > > Can you consider using a switch () case statement here? Does this concept even make sense? The Marvell 10G phy will use SGMII for 10/100/1000Mbs and swap to XGMII for 10Gbps. It however stays a c45 device all the time. In general, i don't think PHY mode is related to C22/C45. Andrew
Hi Andrew, > -----Original Message----- > From: Andrew Lunn [mailto:andrew@lunn.ch] > Sent: Sunday, June 11, 2017 3:43 AM > To: Florian Fainelli > Cc: Salil Mehta; davem@davemloft.net; Zhuangyuzeng (Yisen); huangdaode; > lipeng (Y); mehta.salil.lnk@gmail.com; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org; Linuxarm > Subject: Re: [PATCH net-next 6/9] net: hns3: Add MDIO support to HNS3 > Ethernet driver for hip08 SoC > > > > +int hclge_mac_mdio_config(struct hclge_dev *hdev) > > > +{ > > > + struct hclge_mac *mac = &hdev->hw.mac; > > > + struct mii_bus *mdio_bus; > > > + struct net_device *ndev = &mac->ndev; > > > + struct phy_device *phy; > > > + bool is_c45; > > > + int ret; > > > + > > > + if (hdev->hw.mac.phy_addr >= PHY_MAX_ADDR) > > > + return 0; > > > + > > > + if (hdev->hw.mac.phy_if == PHY_INTERFACE_MODE_NA) > > > + return 0; > > > + else if (mac->phy_if == PHY_INTERFACE_MODE_SGMII) > > > + is_c45 = 0; > > > + else if (mac->phy_if == PHY_INTERFACE_MODE_XGMII) > > > + is_c45 = 1; > > > + else > > > + return -ENODATA; > > > > Can you consider using a switch () case statement here? > > Does this concept even make sense? The Marvell 10G phy will use SGMII > for 10/100/1000Mbs and swap to XGMII for 10Gbps. It however stays a > c45 device all the time. > > In general, i don't think PHY mode is related to C22/C45. You are correct is_c45 should not be derived from the type of PHY like it is being done here. I have changed this code and now we are getting this information from the IMP(Integrated Management Processor) which handles the NIC commands and also maintains Ethernet specific configuration. Thanks Salil > > Andrew
> You are correct is_c45 should not be derived from the type of PHY like > it is being done here. I have changed this code and now we are getting this > information from the IMP(Integrated Management Processor) which handles > the NIC commands and also maintains Ethernet specific configuration. Florian With c22, registering the MDIO bus is enough to cause all c22 addresses to be probed and the PHYs found. If you are not using device tree, you can then use phy_find_first() to get the first phy on the bus. As far as i remember, this does not work for c45. mdiobus_scan() is only looking for c22 PHYs. Maybe we should be making mdiobus_scan() look first for a c45 and then try c22? Andrew
Hi Florian, > -----Original Message----- > From: Florian Fainelli [mailto:f.fainelli@gmail.com] > Sent: Saturday, June 10, 2017 8:04 PM > To: Salil Mehta; davem@davemloft.net > Cc: Zhuangyuzeng (Yisen); huangdaode; lipeng (Y); > mehta.salil.lnk@gmail.com; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org; Linuxarm; Andrew Lunn > Subject: Re: [PATCH net-next 6/9] net: hns3: Add MDIO support to HNS3 > Ethernet driver for hip08 SoC > > Le 06/09/17 à 20:46, Salil Mehta a écrit : > > This patch adds the support of MDIO bus interface for HNS3 driver. > > Code provides various interfaces to start and stop the PHY layer > > and to read and write the MDIO bus or PHY. > > > > Signed-off-by: Daode Huang <huangdaode@hisilicon.com> > > Signed-off-by: lipeng <lipeng321@huawei.com> > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > > Signed-off-by: Yisen Zhuang <yisen.zhuang@huawei.com> > > --- > > .../ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c | 310 > +++++++++++++++++++++ > > 1 file changed, 310 insertions(+) > > create mode 100644 > drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c > > > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c > b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c > > new file mode 100644 > > index 0000000..c6812d2 > > --- /dev/null > > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c > > @@ -0,0 +1,310 @@ > > +/* > > + * Copyright (c) 2016~2017 Hisilicon Limited. > > + * > > + * This program is free software; you can redistribute it and/or > modify > > + * it under the terms of the GNU General Public License as published > by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + */ > > + > > +#include <linux/etherdevice.h> > > +#include <linux/kernel.h> > > + > > +#include "hclge_cmd.h" > > +#include "hclge_main.h" > > + > > +enum hclge_mdio_c22_op_seq { > > + HCLGE_MDIO_C22_WRITE = 1, > > + HCLGE_MDIO_C22_READ = 2 > > +}; > > + > > +enum hclge_mdio_c45_op_seq { > > + HCLGE_MDIO_C45_WRITE_ADDR = 0, > > + HCLGE_MDIO_C45_WRITE_DATA, > > + HCLGE_MDIO_C45_READ_INCREMENT, > > + HCLGE_MDIO_C45_READ > > +}; > > + > > +#define HCLGE_MDIO_CTRL_START_BIT BIT(0) > > +#define HCLGE_MDIO_CTRL_ST_MSK GENMASK(2, 1) > > +#define HCLGE_MDIO_CTRL_ST_LSH 1 > > +#define HCLGE_MDIO_IS_C22(c22) (((c22) << HCLGE_MDIO_CTRL_ST_LSH) & > \ > > + HCLGE_MDIO_CTRL_ST_MSK) > > + > > +#define HCLGE_MDIO_CTRL_OP_MSK GENMASK(4, 3) > > +#define HCLGE_MDIO_CTRL_OP_LSH 3 > > +#define HCLGE_MDIO_CTRL_OP(access) \ > > + (((access) << HCLGE_MDIO_CTRL_OP_LSH) & HCLGE_MDIO_CTRL_OP_MSK) > > +#define HCLGE_MDIO_CTRL_PRTAD_MSK GENMASK(4, 0) > > +#define HCLGE_MDIO_CTRL_DEVAD_MSK GENMASK(4, 0) > > + > > +#define HCLGE_MDIO_STA_VAL(val) ((val) & BIT(0)) > > + > > +struct hclge_mdio_cfg_cmd { > > + u8 ctrl_bit; > > + u8 prtad; /* The external port address */ > > + u8 devad; /* The external device address */ > > + u8 rsvd; > > + __le16 addr_c45;/* Only valid for c45 */ > > + __le16 data_wr; > > + __le16 data_rd; > > + __le16 sta;> +}; > > + > > +static int hclge_mdio_write(struct mii_bus *bus, int phy_id, int > regnum, > > + u16 data) > > +{ > > + struct hclge_dev *hdev = (struct hclge_dev *)bus->priv; > > + struct hclge_mdio_cfg_cmd *mdio_cmd; > > + enum hclge_cmd_status status; > > + struct hclge_desc desc; > > + u8 is_c45, devad; > > + u16 reg; > > + > > + if (!bus) > > + return -EINVAL; > > + > > + is_c45 = !!(regnum & MII_ADDR_C45); > > + devad = ((regnum >> 16) & 0x1f); > > + reg = (u16)(regnum & 0xffff); > > + > > + hclge_cmd_setup_basic_desc(&desc, HCLGE_OPC_MDIO_CONFIG, false); > > + > > + mdio_cmd = (struct hclge_mdio_cfg_cmd *)desc.data; > > + > > + if (!is_c45) { > > It would be more readable with positive logic: if (is_c45) { } else { } Fine, will change. Thanks Salil > > > + /* C22 write reg and data */ > > + mdio_cmd->ctrl_bit = HCLGE_MDIO_IS_C22(!is_c45); > > + mdio_cmd->ctrl_bit |= > HCLGE_MDIO_CTRL_OP(HCLGE_MDIO_C22_WRITE); > > + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_START_BIT; > > + mdio_cmd->data_wr = cpu_to_le16(data); > > + mdio_cmd->devad = devad & HCLGE_MDIO_CTRL_DEVAD_MSK; > > + mdio_cmd->prtad = phy_id & HCLGE_MDIO_CTRL_PRTAD_MSK; > > + } else { > > + /* Set phy addr */ > > + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_START_BIT; > > + mdio_cmd->addr_c45 = cpu_to_le16(reg); > > + mdio_cmd->data_wr = cpu_to_le16(data); > > + mdio_cmd->devad = devad & HCLGE_MDIO_CTRL_DEVAD_MSK; > > + mdio_cmd->prtad = phy_id & HCLGE_MDIO_CTRL_PRTAD_MSK; > > + } > > There is some common initialization that you could probably extracted > out of the C22/C45 clause here. Yes, I agree. Will remove the redundancy and take out the common part. Thanks Salil > > > + > > + status = hclge_cmd_send(&hdev->hw, &desc, 1); > > + if (status) { > > + dev_err(&hdev->pdev->dev, > > + "mdio write fail when sending cmd, status is %d.\n", > > + status); > > + return -EIO; > > + } > > + > > + return 0; > > +} > > + > > +static int hclge_mdio_read(struct mii_bus *bus, int phy_id, int > regnum) > > +{ > > + struct hclge_dev *hdev = (struct hclge_dev *)bus->priv; > > + struct hclge_mdio_cfg_cmd *mdio_cmd; > > + enum hclge_cmd_status status; > > + struct hclge_desc desc; > > + u8 is_c45, devad; > > + u16 reg; > > + > > + if (!bus) > > + return -EINVAL; > > + > > + is_c45 = !!(regnum & MII_ADDR_C45); > > + devad = ((regnum >> 16) & GENMASK(4, 0)); > > + reg = (u16)(regnum & GENMASK(15, 0)); > > + > > + hclge_cmd_setup_basic_desc(&desc, HCLGE_OPC_MDIO_CONFIG, true); > > + > > + mdio_cmd = (struct hclge_mdio_cfg_cmd *)desc.data; > > + > > + dev_dbg(&bus->dev, "phy id=%d, is_c45=%d, devad=%d, reg=%#x!\n", > > + phy_id, is_c45, devad, reg); > > + > > + if (!is_c45) { > > + /* C22 read reg */ > > + mdio_cmd->ctrl_bit = HCLGE_MDIO_IS_C22(!is_c45); > > + mdio_cmd->ctrl_bit |= > HCLGE_MDIO_CTRL_OP(HCLGE_MDIO_C22_READ); > > + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_START_BIT; > > + mdio_cmd->devad = reg & HCLGE_MDIO_CTRL_DEVAD_MSK; > > + mdio_cmd->prtad = phy_id & HCLGE_MDIO_CTRL_PRTAD_MSK; > > + } else { > > + /* C45 phy addr */ > > + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_START_BIT; > > + > > + mdio_cmd->addr_c45 = cpu_to_le16(reg); > > + mdio_cmd->devad = devad & HCLGE_MDIO_CTRL_DEVAD_MSK; > > + mdio_cmd->prtad = phy_id & HCLGE_MDIO_CTRL_PRTAD_MSK; > > + } > > Same here, the last two assignments could be common with an appropriate > variable assignment before. Yes, agreed. Will change this as well. Thanks Salil > > > + > > + /* Read out phy data */ > > + status = hclge_cmd_send(&hdev->hw, &desc, 1); > > + if (status) { > > + dev_err(&hdev->pdev->dev, > > + "mdio read fail when get data, status is %d.\n", > > + status); > > + return -EIO; > > + } > > + > > + if (le16_to_cpu(HCLGE_MDIO_STA_VAL(mdio_cmd->sta))) { > > + dev_err(&hdev->pdev->dev, "mdio read data error\n"); > > + return -ENOMEM; > > + } > > + > > + return le16_to_cpu(mdio_cmd->data_rd); > > +} > > + > > +int hclge_mac_mdio_config(struct hclge_dev *hdev) > > +{ > > + struct hclge_mac *mac = &hdev->hw.mac; > > + struct mii_bus *mdio_bus; > > + struct net_device *ndev = &mac->ndev; > > + struct phy_device *phy; > > + bool is_c45; > > + int ret; > > + > > + if (hdev->hw.mac.phy_addr >= PHY_MAX_ADDR) > > + return 0; > > + > > + if (hdev->hw.mac.phy_if == PHY_INTERFACE_MODE_NA) > > + return 0; > > + else if (mac->phy_if == PHY_INTERFACE_MODE_SGMII) > > + is_c45 = 0; > > + else if (mac->phy_if == PHY_INTERFACE_MODE_XGMII) > > + is_c45 = 1; > > + else > > + return -ENODATA; > > Can you consider using a switch () case statement here? This needs to be removed and will get information, if the MAC/MDIO supports Clause 45 from the IMP (Integrated Management Processor). Thanks Salil > > > + > > + SET_NETDEV_DEV(ndev, &hdev->pdev->dev); > > + > > + mdio_bus = devm_mdiobus_alloc(&hdev->pdev->dev); > > + if (!mdio_bus) { > > + ret = -ENOMEM; > > + goto err_miibus_alloc; > > + } > > + > > + mdio_bus->name = "hisilicon MII bus"; > > + mdio_bus->read = hclge_mdio_read; > > + mdio_bus->write = hclge_mdio_write; > > + snprintf(mdio_bus->id, MII_BUS_ID_SIZE, "%s-%s", "mii", > > + dev_name(&hdev->pdev->dev)); > > + > > + mdio_bus->parent = &hdev->pdev->dev; > > + mdio_bus->priv = hdev; > > + mdio_bus->phy_mask = ~0; > > + ret = mdiobus_register(mdio_bus); > > + if (ret) { > > + dev_err(mdio_bus->parent, > > + "Failed to register MDIO bus ret = %#x\n", ret); > > + goto err_mdio_register; > > + } > > + > > + phy = get_phy_device(mdio_bus, mac->phy_addr, is_c45); > > + if (!phy || IS_ERR(phy)) { > > + dev_err(mdio_bus->parent, "Failed to get phy device\n"); > > + ret = -EIO; > > + goto err_mdio_register; > > + } > > Hum why do you do this? mdiobus_register() will scan through your bus > provided that you set an appropriate phy_mask value (here you tell it > not to) and you already provide the PHY address to scan for > I know this looks weird but the reason why it appears as it is in code is: mdiobus_register() calls mdiobus_scan(). If you see below code leg function get_phy_device() assumes to be having supporting Clause 22 so its input parameter 'is_c45' is always 'false'. struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr) { struct phy_device *phydev; int err; phydev = get_phy_device(bus, addr, false); if (IS_ERR(phydev)) ^^^^^ return phydev; [...] } Therefore, to support C45 device we did below: * disabled the autoscan/mdiobus_scan() Of the PHY devices using the phy_mask(= ~0) * Now, did almost the same thing what mdiobus_scan does i.e. * get_phy_device but with is_c45 (=true/false) * register the above phy device with phy_device_register() There could be some gap in my understanding, please help to correct this? Thanks Salil > > + > > + phy->irq = mdio_bus->irq[mac->phy_addr]; > > + > > + /* All data is now stored in the phy struct; > > + * register it > > + */ > > + ret = phy_device_register(phy); > > + if (ret) { > > + ret = -ENODEV; > > + goto err_phy_register; > > + } > > This also looks completely unecessary if you correctly set phy_mask. > Can you explain why you did that way? Yes, bit weird but explained you the reason above. Please help me in understanding this better. I will change the code for the better! Thanks Salil > > > + > > + mac->phy_dev = phy; > > + > > + return 0; > > + > > +err_phy_register: > > + phy_device_free(phy); > > +err_mdio_register: > > + mdiobus_unregister(mdio_bus); > > + mdiobus_free(mdio_bus); > > +err_miibus_alloc: > > + return ret; > > +} > > + > > +static void hclge_mac_adjust_link(struct net_device *net_dev) > > +{ > > + int duplex; > > + int speed; > > + struct hclge_mac *hw_mac; > > + struct hclge_hw *hw; > > + struct hclge_dev *hdev; > > + > > + if (!net_dev) > > + return; > > + > > + hw_mac = container_of(net_dev, struct hclge_mac, ndev); > > + hw = container_of(hw_mac, struct hclge_hw, mac); > > + hdev = hw->back; > > + > > + speed = hw_mac->phy_dev->speed; > > + duplex = hw_mac->phy_dev->duplex; > > + > > + /* update antoneg. */ > > + hw_mac->autoneg = hw_mac->phy_dev->autoneg; > > + > > + if ((hw_mac->speed != speed) || (hw_mac->duplex != duplex)) > > + (void)hclge_cfg_mac_speed_dup(hdev, speed, !!duplex); > > +} > > + > > +int hclge_mac_start_phy(struct hclge_dev *hdev) > > +{ > > + struct hclge_mac *mac = &hdev->hw.mac; > > + struct phy_device *phy_dev = mac->phy_dev; > > + int ret; > > + > > + if (!phy_dev) > > + return 0; > > + > > + if (mac->phy_if != PHY_INTERFACE_MODE_XGMII) { > > + phy_dev->dev_flags = 0; > > + > > + ret = phy_connect_direct(&mac->ndev, phy_dev, > > + hclge_mac_adjust_link, > > + mac->phy_if); > > + phy_dev->supported = SUPPORTED_10baseT_Half | > > + SUPPORTED_10baseT_Full | > > + SUPPORTED_100baseT_Half | > > + SUPPORTED_100baseT_Full | > > + SUPPORTED_Autoneg | > > + SUPPORTED_1000baseT_Full; > > + > > + phy_dev->autoneg = false; > > + } else { > > + ret = phy_attach_direct(&mac->ndev, phy_dev, 0, mac- > >phy_if); > > + phy_dev->supported = SUPPORTED_10000baseR_FEC | > > + SUPPORTED_10000baseKR_Full; > > + } > > Why is there a different PHYLIB call based on XGMII or not? Cannot you > always attach to the PHY and then start the PHY state machine instead > of doing a connect() or attach() here? > I think we can. Maybe, I will consider fixing this in subsequent patches I float. Thanks Salil > > + if (unlikely(ret)) > > + return -ENODEV; > > + > > + phy_start(phy_dev); > > + > > + return 0; > > +} > > + > > +void hclge_mac_stop_phy(struct hclge_dev *hdev) > > +{ > > + struct hclge_mac *mac = &hdev->hw.mac; > > + struct phy_device *phy_dev = mac->phy_dev; > > + > > + if (!phy_dev) > > + return; > > + > > + phy_stop(phy_dev); > > + > > + if (mac->phy_if != PHY_INTERFACE_MODE_XGMII) > > + phy_disconnect(phy_dev); > > + else > > + phy_detach(phy_dev); > > You cannot detach or disconnect based on the PHY interface, that does > not really make sense as both are not symetrical. Yes, you are correct. I think will try to fix this as well in subsequent patches. Thanks Salil > -- > Florian
> > Hum why do you do this? mdiobus_register() will scan through your bus > > provided that you set an appropriate phy_mask value (here you tell it > > not to) and you already provide the PHY address to scan for > > > I know this looks weird but the reason why it appears as it is in code is: > > mdiobus_register() calls mdiobus_scan(). If you see below code leg function > get_phy_device() assumes to be having supporting Clause 22 so its input > parameter 'is_c45' is always 'false'. > > struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr) > { > struct phy_device *phydev; > int err; > > phydev = get_phy_device(bus, addr, false); > if (IS_ERR(phydev)) ^^^^^ > return phydev; > [...] > } > > Therefore, to support C45 device we did below: > > * disabled the autoscan/mdiobus_scan() Of the PHY devices using the > phy_mask(= ~0) > * Now, did almost the same thing what mdiobus_scan does i.e. > * get_phy_device but with is_c45 (=true/false) > * register the above phy device with phy_device_register() > > There could be some gap in my understanding, please help to correct this? So this is the question i was asking Florian Rather than hack around limitations of the core, you should fix the core. I think we should make the core first try probing using c45. If that comes back with an error, or does not find a device, try the probe using c22. Andrew
> Since I would be touching the core, lots of drivers will get impacted and will > have to wait till everyone gives clean signal. This will impact our internal > deadlines. But as I said I am eager to cooperate & contribute :) Just as an FYI. Your internal deadlines are irrelevant. We will reject your patches until we are happy with it. We probably won't accept a hack around a core feature limitation. We will want you to fix the core limitation and then do it right in the driver. Andrew
Hi Andrew > -----Original Message----- > From: Andrew Lunn [mailto:andrew@lunn.ch] > Sent: Wednesday, June 14, 2017 2:10 AM > To: Salil Mehta > Cc: Florian Fainelli; davem@davemloft.net; Zhuangyuzeng (Yisen); > huangdaode; lipeng (Y); mehta.salil.lnk@gmail.com; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Linuxarm > Subject: Re: [PATCH net-next 6/9] net: hns3: Add MDIO support to HNS3 > Ethernet driver for hip08 SoC > > > Since I would be touching the core, lots of drivers will get impacted > and will > > have to wait till everyone gives clean signal. This will impact our > internal > > deadlines. But as I said I am eager to cooperate & contribute :) > > Just as an FYI. > > Your internal deadlines are irrelevant. We will reject your patches > until we are happy with it. We probably won't accept a hack around a > core feature limitation. We will want you to fix the core limitation > and then do it right in the driver. > > Andrew We are dropping the support of C45 in our driver till we fix the issue just commented upon. I will try to fix this issue and would soon float a separate patch for that. Later, we shall re-introduce the support of C45 in our HNS3 driver once the PHY core fix has been reviewed and accepted. Thanks Salil
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c new file mode 100644 index 0000000..c6812d2 --- /dev/null +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c @@ -0,0 +1,310 @@ +/* + * Copyright (c) 2016~2017 Hisilicon Limited. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/etherdevice.h> +#include <linux/kernel.h> + +#include "hclge_cmd.h" +#include "hclge_main.h" + +enum hclge_mdio_c22_op_seq { + HCLGE_MDIO_C22_WRITE = 1, + HCLGE_MDIO_C22_READ = 2 +}; + +enum hclge_mdio_c45_op_seq { + HCLGE_MDIO_C45_WRITE_ADDR = 0, + HCLGE_MDIO_C45_WRITE_DATA, + HCLGE_MDIO_C45_READ_INCREMENT, + HCLGE_MDIO_C45_READ +}; + +#define HCLGE_MDIO_CTRL_START_BIT BIT(0) +#define HCLGE_MDIO_CTRL_ST_MSK GENMASK(2, 1) +#define HCLGE_MDIO_CTRL_ST_LSH 1 +#define HCLGE_MDIO_IS_C22(c22) (((c22) << HCLGE_MDIO_CTRL_ST_LSH) & \ + HCLGE_MDIO_CTRL_ST_MSK) + +#define HCLGE_MDIO_CTRL_OP_MSK GENMASK(4, 3) +#define HCLGE_MDIO_CTRL_OP_LSH 3 +#define HCLGE_MDIO_CTRL_OP(access) \ + (((access) << HCLGE_MDIO_CTRL_OP_LSH) & HCLGE_MDIO_CTRL_OP_MSK) +#define HCLGE_MDIO_CTRL_PRTAD_MSK GENMASK(4, 0) +#define HCLGE_MDIO_CTRL_DEVAD_MSK GENMASK(4, 0) + +#define HCLGE_MDIO_STA_VAL(val) ((val) & BIT(0)) + +struct hclge_mdio_cfg_cmd { + u8 ctrl_bit; + u8 prtad; /* The external port address */ + u8 devad; /* The external device address */ + u8 rsvd; + __le16 addr_c45;/* Only valid for c45 */ + __le16 data_wr; + __le16 data_rd; + __le16 sta; +}; + +static int hclge_mdio_write(struct mii_bus *bus, int phy_id, int regnum, + u16 data) +{ + struct hclge_dev *hdev = (struct hclge_dev *)bus->priv; + struct hclge_mdio_cfg_cmd *mdio_cmd; + enum hclge_cmd_status status; + struct hclge_desc desc; + u8 is_c45, devad; + u16 reg; + + if (!bus) + return -EINVAL; + + is_c45 = !!(regnum & MII_ADDR_C45); + devad = ((regnum >> 16) & 0x1f); + reg = (u16)(regnum & 0xffff); + + hclge_cmd_setup_basic_desc(&desc, HCLGE_OPC_MDIO_CONFIG, false); + + mdio_cmd = (struct hclge_mdio_cfg_cmd *)desc.data; + + if (!is_c45) { + /* C22 write reg and data */ + mdio_cmd->ctrl_bit = HCLGE_MDIO_IS_C22(!is_c45); + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_OP(HCLGE_MDIO_C22_WRITE); + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_START_BIT; + mdio_cmd->data_wr = cpu_to_le16(data); + mdio_cmd->devad = devad & HCLGE_MDIO_CTRL_DEVAD_MSK; + mdio_cmd->prtad = phy_id & HCLGE_MDIO_CTRL_PRTAD_MSK; + } else { + /* Set phy addr */ + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_START_BIT; + mdio_cmd->addr_c45 = cpu_to_le16(reg); + mdio_cmd->data_wr = cpu_to_le16(data); + mdio_cmd->devad = devad & HCLGE_MDIO_CTRL_DEVAD_MSK; + mdio_cmd->prtad = phy_id & HCLGE_MDIO_CTRL_PRTAD_MSK; + } + + status = hclge_cmd_send(&hdev->hw, &desc, 1); + if (status) { + dev_err(&hdev->pdev->dev, + "mdio write fail when sending cmd, status is %d.\n", + status); + return -EIO; + } + + return 0; +} + +static int hclge_mdio_read(struct mii_bus *bus, int phy_id, int regnum) +{ + struct hclge_dev *hdev = (struct hclge_dev *)bus->priv; + struct hclge_mdio_cfg_cmd *mdio_cmd; + enum hclge_cmd_status status; + struct hclge_desc desc; + u8 is_c45, devad; + u16 reg; + + if (!bus) + return -EINVAL; + + is_c45 = !!(regnum & MII_ADDR_C45); + devad = ((regnum >> 16) & GENMASK(4, 0)); + reg = (u16)(regnum & GENMASK(15, 0)); + + hclge_cmd_setup_basic_desc(&desc, HCLGE_OPC_MDIO_CONFIG, true); + + mdio_cmd = (struct hclge_mdio_cfg_cmd *)desc.data; + + dev_dbg(&bus->dev, "phy id=%d, is_c45=%d, devad=%d, reg=%#x!\n", + phy_id, is_c45, devad, reg); + + if (!is_c45) { + /* C22 read reg */ + mdio_cmd->ctrl_bit = HCLGE_MDIO_IS_C22(!is_c45); + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_OP(HCLGE_MDIO_C22_READ); + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_START_BIT; + mdio_cmd->devad = reg & HCLGE_MDIO_CTRL_DEVAD_MSK; + mdio_cmd->prtad = phy_id & HCLGE_MDIO_CTRL_PRTAD_MSK; + } else { + /* C45 phy addr */ + mdio_cmd->ctrl_bit |= HCLGE_MDIO_CTRL_START_BIT; + + mdio_cmd->addr_c45 = cpu_to_le16(reg); + mdio_cmd->devad = devad & HCLGE_MDIO_CTRL_DEVAD_MSK; + mdio_cmd->prtad = phy_id & HCLGE_MDIO_CTRL_PRTAD_MSK; + } + + /* Read out phy data */ + status = hclge_cmd_send(&hdev->hw, &desc, 1); + if (status) { + dev_err(&hdev->pdev->dev, + "mdio read fail when get data, status is %d.\n", + status); + return -EIO; + } + + if (le16_to_cpu(HCLGE_MDIO_STA_VAL(mdio_cmd->sta))) { + dev_err(&hdev->pdev->dev, "mdio read data error\n"); + return -ENOMEM; + } + + return le16_to_cpu(mdio_cmd->data_rd); +} + +int hclge_mac_mdio_config(struct hclge_dev *hdev) +{ + struct hclge_mac *mac = &hdev->hw.mac; + struct mii_bus *mdio_bus; + struct net_device *ndev = &mac->ndev; + struct phy_device *phy; + bool is_c45; + int ret; + + if (hdev->hw.mac.phy_addr >= PHY_MAX_ADDR) + return 0; + + if (hdev->hw.mac.phy_if == PHY_INTERFACE_MODE_NA) + return 0; + else if (mac->phy_if == PHY_INTERFACE_MODE_SGMII) + is_c45 = 0; + else if (mac->phy_if == PHY_INTERFACE_MODE_XGMII) + is_c45 = 1; + else + return -ENODATA; + + SET_NETDEV_DEV(ndev, &hdev->pdev->dev); + + mdio_bus = devm_mdiobus_alloc(&hdev->pdev->dev); + if (!mdio_bus) { + ret = -ENOMEM; + goto err_miibus_alloc; + } + + mdio_bus->name = "hisilicon MII bus"; + mdio_bus->read = hclge_mdio_read; + mdio_bus->write = hclge_mdio_write; + snprintf(mdio_bus->id, MII_BUS_ID_SIZE, "%s-%s", "mii", + dev_name(&hdev->pdev->dev)); + + mdio_bus->parent = &hdev->pdev->dev; + mdio_bus->priv = hdev; + mdio_bus->phy_mask = ~0; + ret = mdiobus_register(mdio_bus); + if (ret) { + dev_err(mdio_bus->parent, + "Failed to register MDIO bus ret = %#x\n", ret); + goto err_mdio_register; + } + + phy = get_phy_device(mdio_bus, mac->phy_addr, is_c45); + if (!phy || IS_ERR(phy)) { + dev_err(mdio_bus->parent, "Failed to get phy device\n"); + ret = -EIO; + goto err_mdio_register; + } + + phy->irq = mdio_bus->irq[mac->phy_addr]; + + /* All data is now stored in the phy struct; + * register it + */ + ret = phy_device_register(phy); + if (ret) { + ret = -ENODEV; + goto err_phy_register; + } + + mac->phy_dev = phy; + + return 0; + +err_phy_register: + phy_device_free(phy); +err_mdio_register: + mdiobus_unregister(mdio_bus); + mdiobus_free(mdio_bus); +err_miibus_alloc: + return ret; +} + +static void hclge_mac_adjust_link(struct net_device *net_dev) +{ + int duplex; + int speed; + struct hclge_mac *hw_mac; + struct hclge_hw *hw; + struct hclge_dev *hdev; + + if (!net_dev) + return; + + hw_mac = container_of(net_dev, struct hclge_mac, ndev); + hw = container_of(hw_mac, struct hclge_hw, mac); + hdev = hw->back; + + speed = hw_mac->phy_dev->speed; + duplex = hw_mac->phy_dev->duplex; + + /* update antoneg. */ + hw_mac->autoneg = hw_mac->phy_dev->autoneg; + + if ((hw_mac->speed != speed) || (hw_mac->duplex != duplex)) + (void)hclge_cfg_mac_speed_dup(hdev, speed, !!duplex); +} + +int hclge_mac_start_phy(struct hclge_dev *hdev) +{ + struct hclge_mac *mac = &hdev->hw.mac; + struct phy_device *phy_dev = mac->phy_dev; + int ret; + + if (!phy_dev) + return 0; + + if (mac->phy_if != PHY_INTERFACE_MODE_XGMII) { + phy_dev->dev_flags = 0; + + ret = phy_connect_direct(&mac->ndev, phy_dev, + hclge_mac_adjust_link, + mac->phy_if); + phy_dev->supported = SUPPORTED_10baseT_Half | + SUPPORTED_10baseT_Full | + SUPPORTED_100baseT_Half | + SUPPORTED_100baseT_Full | + SUPPORTED_Autoneg | + SUPPORTED_1000baseT_Full; + + phy_dev->autoneg = false; + } else { + ret = phy_attach_direct(&mac->ndev, phy_dev, 0, mac->phy_if); + phy_dev->supported = SUPPORTED_10000baseR_FEC | + SUPPORTED_10000baseKR_Full; + } + if (unlikely(ret)) + return -ENODEV; + + phy_start(phy_dev); + + return 0; +} + +void hclge_mac_stop_phy(struct hclge_dev *hdev) +{ + struct hclge_mac *mac = &hdev->hw.mac; + struct phy_device *phy_dev = mac->phy_dev; + + if (!phy_dev) + return; + + phy_stop(phy_dev); + + if (mac->phy_if != PHY_INTERFACE_MODE_XGMII) + phy_disconnect(phy_dev); + else + phy_detach(phy_dev); +}