| 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: <20201223012357.GS3107610@lunn.ch>
Date: Wed, 23 Dec 2020 02:23:57 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Robert Marko <robert.marko@...tura.hr>
Cc: agross@...nel.org, bjorn.andersson@...aro.org, robh+dt@...nel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
netdev@...r.kernel.org, linux@...linux.org.uk,
Luka Perkov <luka.perkov@...tura.hr>
Subject: Re: [PATCH 3/4] net: phy: Add Qualcomm QCA807x driver
> +++ b/drivers/net/phy/Kconfig
> @@ -264,6 +264,16 @@ config QSEMI_PHY
> help
> Currently supports the qs6612
>
> +config QCA807X_PHY
> + tristate "Qualcomm QCA807X PHYs"
Kconfig is sorted based on the tristate string. So this should be
after AT803X_PHY.
> + depends on OF_MDIO
> + help
> + Adds support for the Qualcomm QCA807x PHYs.
> + These are 802.3 Clause 22 compliant PHYs supporting gigabit
> + ethernet as well as 100Base-FX and 1000Base-X fibre.
> +
> + Currently supports the QCA8072 and QCA8075 models.
> +
> config REALTEK_PHY
> tristate "Realtek PHYs"
> help
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index ca0a313423b9..5f463ce0f711 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -74,6 +74,7 @@ obj-$(CONFIG_MICROSEMI_PHY) += mscc/
> obj-$(CONFIG_NATIONAL_PHY) += national.o
> obj-$(CONFIG_NXP_TJA11XX_PHY) += nxp-tja11xx.o
> obj-$(CONFIG_QSEMI_PHY) += qsemi.o
> +obj-$(CONFIG_QCA807X_PHY) += qca807x.o
> obj-$(CONFIG_REALTEK_PHY) += realtek.o
One line earlier please.
> +static int qca807x_cable_test_report_trans(int result)
> +{
> + switch (result) {
> + case QCA807X_CDT_RESULTS_OK:
> + return ETHTOOL_A_CABLE_RESULT_CODE_OK;
> + case QCA807X_CDT_RESULTS_OPEN:
> + return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
> + case QCA807X_CDT_RESULTS_SAME_SHORT:
> + return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
> + case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_OK:
> + case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI2_SAME_OK:
> + case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI3_SAME_OK:
> + case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_OPEN:
> + case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI2_SAME_OPEN:
> + case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI3_SAME_OPEN:
> + case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_SHORT:
> + case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI2_SAME_SHORT:
> + case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI3_SAME_SHORT:
Feel free to add an optional pair to the netlink message, indicating
which pair this pair is shorted to.
> + return ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT;
> + default:
> + return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
> + }
> +}
> +static int qca807x_cable_test_start(struct phy_device *phydev)
> +{
> + int val, ret;
> +
> + val = phy_read(phydev, QCA807X_CDT);
> + /* Enable inter-pair short check as well */
> + val &= ~QCA807X_CDT_ENABLE_INTER_PAIR_SHORT;
> + val |= QCA807X_CDT_ENABLE;
> + ret = phy_write(phydev, QCA807X_CDT, val);
What happens when you are using the PHY as a media converted to Fibre?
Should we return -EOPNOTSUPP here? I'm assuming it cannot do cable
test on a fibre pair.
> +static int qca807x_read_fiber_status(struct phy_device *phydev, bool combo_port)
> +{
> + int ss, err, page, lpa, old_link = phydev->link;
> +
> + /* Check whether fiber page is set and set if needed */
> + page = phy_read(phydev, QCA807X_CHIP_CONFIGURATION);
> + if (page & QCA807X_BT_BX_REG_SEL) {
> + page &= ~QCA807X_BT_BX_REG_SEL;
> + phy_write(phydev, QCA807X_CHIP_CONFIGURATION, page);
> + }
> +
> + /* Update the link, but return if there was an error */
> + err = genphy_update_link(phydev);
> + if (err)
> + return err;
> +
> + /* why bother the PHY if nothing can have changed */
> + if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
> + return 0;
> +
> + phydev->speed = SPEED_UNKNOWN;
> + phydev->duplex = DUPLEX_UNKNOWN;
> + phydev->pause = 0;
> + phydev->asym_pause = 0;
> +
> + if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
> + lpa = phy_read(phydev, MII_LPA);
> + if (lpa < 0)
> + return lpa;
> +
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> + phydev->lp_advertising, lpa & LPA_LPACK);
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> + phydev->lp_advertising, lpa & LPA_1000XFULL);
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> + phydev->lp_advertising, lpa & LPA_1000XPAUSE);
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> + phydev->lp_advertising,
> + lpa & LPA_1000XPAUSE_ASYM);
> +
> + phy_resolve_aneg_linkmode(phydev);
> + }
This looks a lot like genphy_c37_read_status(). You could call it, and
then over write the speed and duplex with values from the PHY specific
registers.
> +static int qca807x_config_intr(struct phy_device *phydev)
> +{
> + int ret, val;
> +
> + val = phy_read(phydev, QCA807X_INTR_ENABLE);
> +
> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> + /* Check for combo port as it has fewer interrupts */
> + if (phy_read(phydev, QCA807X_CHIP_CONFIGURATION)) {
> + val |= QCA807X_INTR_ENABLE_SPEED_CHANGED;
> + val |= QCA807X_INTR_ENABLE_LINK_FAIL;
> + val |= QCA807X_INTR_ENABLE_LINK_SUCCESS;
> + } else {
> + val |= QCA807X_INTR_ENABLE_AUTONEG_ERR;
> + val |= QCA807X_INTR_ENABLE_SPEED_CHANGED;
> + val |= QCA807X_INTR_ENABLE_DUPLEX_CHANGED;
> + val |= QCA807X_INTR_ENABLE_LINK_FAIL;
> + val |= QCA807X_INTR_ENABLE_LINK_SUCCESS;
> + }
> + ret = phy_write(phydev, QCA807X_INTR_ENABLE, val);
> + } else {
> + ret = phy_write(phydev, QCA807X_INTR_ENABLE, 0);
> + }
> +
> + return ret;
> +}
> +
> +static int qca807x_ack_intr(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = phy_read(phydev, QCA807X_INTR_STATUS);
> +
> + return (ret < 0) ? ret : 0;
> +}
You need to rework this to follow what Ioana has done for interrupt
handling.
> + {
> + PHY_ID_MATCH_EXACT(PHY_ID_QCA8075),
> + .name = "Qualcomm QCA8075",
> + .flags = PHY_POLL_CABLE_TEST,
> + /* PHY_GBIT_FEATURES */
> + .probe = qca807x_probe,
> + .config_init = qca807x_config,
> + .read_status = qca807x_read_status,
> + .config_intr = qca807x_config_intr,
> + .ack_interrupt = qca807x_ack_intr,
> + .soft_reset = genphy_soft_reset,
> + .get_tunable = qca807x_get_tunable,
> + .set_tunable = qca807x_set_tunable,
> + .cable_test_start = qca807x_cable_test_start,
> + .cable_test_get_status = qca807x_cable_test_get_status,
> + },
> + {
> + PHY_ID_MATCH_EXACT(PHY_ID_QCA807X_PSGMII),
> + .name = "Qualcomm QCA807x PSGMII",
> + .probe = qca807x_psgmii_config,
This looks odd.
Andrew
Powered by blists - more mailing lists