Add support for Airoha AN8855 Internal Switch Gigabit PHY.
This is a simple PHY driver to configure and calibrate the PHY for the
AN8855 Switch with the use of NVMEM cells.
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
MAINTAINERS | 1 +
drivers/net/phy/Kconfig | 5 +
drivers/net/phy/Makefile | 1 +
drivers/net/phy/air_an8855.c | 278 +++++++++++++++++++++++++++++++++++
4 files changed, 285 insertions(+)
create mode 100644 drivers/net/phy/air_an8855.c
diff --git a/MAINTAINERS b/MAINTAINERS
index e3077d9feee2..cf34add2a0bb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -726,6 +726,7 @@ S: Maintained
F: Documentation/devicetree/bindings/net/dsa/airoha,an8855.yaml
F: drivers/net/dsa/an8855.c
F: drivers/net/dsa/an8855.h
+F: drivers/net/phy/air_an8855.c
AIROHA ETHERNET DRIVER
M: Lorenzo Bianconi <lorenzo@kernel.org>
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index ee3ea0b56d48..1d474038ea7f 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -79,6 +79,11 @@ config SFP
comment "MII PHY device drivers"
+config AIR_AN8855_PHY
+ tristate "Airoha AN8855 Internal Gigabit PHY"
+ help
+ Currently supports the internal Airoha AN8855 Switch PHY.
+
config AIR_EN8811H_PHY
tristate "Airoha EN8811H 2.5 Gigabit PHY"
help
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 90f886844381..baba7894785b 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -35,6 +35,7 @@ obj-y += $(sfp-obj-y) $(sfp-obj-m)
obj-$(CONFIG_ADIN_PHY) += adin.o
obj-$(CONFIG_ADIN1100_PHY) += adin1100.o
+obj-$(CONFIG_AIR_AN8855_PHY) += air_an8855.o
obj-$(CONFIG_AIR_EN8811H_PHY) += air_en8811h.o
obj-$(CONFIG_AMD_PHY) += amd.o
obj-$(CONFIG_AMCC_QT2025_PHY) += qt2025.o
diff --git a/drivers/net/phy/air_an8855.c b/drivers/net/phy/air_an8855.c
new file mode 100644
index 000000000000..0c518fd62f88
--- /dev/null
+++ b/drivers/net/phy/air_an8855.c
@@ -0,0 +1,278 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2024 Christian Marangi <ansuelsmth@gmail.com>
+ */
+
+#include <linux/phy.h>
+#include <linux/module.h>
+#include <linux/bitfield.h>
+#include <linux/nvmem-consumer.h>
+
+#define AN8855_PHY_SELECT_PAGE 0x1f
+/* Mask speculation based on page up to 0x4 */
+#define AN8855_PHY_PAGE GENMASK(2, 0)
+#define AN8855_PHY_PAGE_STANDARD FIELD_PREP_CONST(AN8855_PHY_PAGE, 0x0)
+#define AN8855_PHY_PAGE_EXTENDED_1 FIELD_PREP_CONST(AN8855_PHY_PAGE, 0x1)
+
+/* MII Registers Page 1 */
+#define AN8855_PHY_EXT_REG_14 0x14
+#define AN8855_PHY_EN_DOWN_SHFIT BIT(4)
+
+/* R50 Calibration regs in MDIO_MMD_VEND1 */
+#define AN8855_PHY_R500HM_RSEL_TX_AB 0x174
+#define AN8855_PHY_R50OHM_RSEL_TX_A_EN BIT(15)
+#define AN8855_PHY_R50OHM_RSEL_TX_A GENMASK(14, 8)
+#define AN8855_PHY_R50OHM_RSEL_TX_B_EN BIT(7)
+#define AN8855_PHY_R50OHM_RSEL_TX_B GENMASK(6, 0)
+#define AN8855_PHY_R500HM_RSEL_TX_CD 0x175
+#define AN8855_PHY_R50OHM_RSEL_TX_C_EN BIT(15)
+#define AN8855_PHY_R50OHM_RSEL_TX_C GENMASK(14, 8)
+#define AN8855_PHY_R50OHM_RSEL_TX_D_EN BIT(7)
+#define AN8855_PHY_R50OHM_RSEL_TX_D GENMASK(6, 0)
+
+#define AN8855_SWITCH_EFUSE_R50O GENMASK(30, 24)
+
+/* PHY TX PAIR DELAY SELECT Register */
+#define AN8855_PHY_TX_PAIR_DLY_SEL_GBE 0x013
+#define AN8855_PHY_CR_DA_TX_PAIR_DELKAY_SEL_A_GBE GENMASK(14, 12)
+#define AN8855_PHY_CR_DA_TX_PAIR_DELKAY_SEL_B_GBE GENMASK(10, 8)
+#define AN8855_PHY_CR_DA_TX_PAIR_DELKAY_SEL_C_GBE GENMASK(6, 4)
+#define AN8855_PHY_CR_DA_TX_PAIR_DELKAY_SEL_D_GBE GENMASK(2, 0)
+/* PHY ADC Register */
+#define AN8855_PHY_RXADC_CTRL 0x0d8
+#define AN8855_PHY_RG_AD_SAMNPLE_PHSEL_A BIT(12)
+#define AN8855_PHY_RG_AD_SAMNPLE_PHSEL_B BIT(8)
+#define AN8855_PHY_RG_AD_SAMNPLE_PHSEL_C BIT(4)
+#define AN8855_PHY_RG_AD_SAMNPLE_PHSEL_D BIT(0)
+#define AN8855_PHY_RXADC_REV_0 0x0d9
+#define AN8855_PHY_RG_AD_RESERVE0_A GENMASK(15, 8)
+#define AN8855_PHY_RG_AD_RESERVE0_B GENMASK(7, 0)
+#define AN8855_PHY_RXADC_REV_1 0x0da
+#define AN8855_PHY_RG_AD_RESERVE0_C GENMASK(15, 8)
+#define AN8855_PHY_RG_AD_RESERVE0_D GENMASK(7, 0)
+
+#define AN8855_PHY_ID 0xc0ff0410
+
+struct air_an8855_priv {
+ u8 calibration_data[4];
+};
+
+static const u8 dsa_r50ohm_table[] = {
+ 127, 127, 127, 127, 127, 127, 127, 127, 127, 127,
+ 127, 127, 127, 127, 127, 127, 127, 126, 122, 117,
+ 112, 109, 104, 101, 97, 94, 90, 88, 84, 80,
+ 78, 74, 72, 68, 66, 64, 61, 58, 56, 53,
+ 51, 48, 47, 44, 42, 40, 38, 36, 34, 32,
+ 31, 28, 27, 24, 24, 22, 20, 18, 16, 16,
+ 14, 12, 11, 9
+};
+
+static int en8855_get_r50ohm_val(struct device *dev, const char *calib_name,
+ u8 *dest)
+{
+ u32 shift_sel, val;
+ int ret;
+ int i;
+
+ ret = nvmem_cell_read_u32(dev, calib_name, &val);
+ if (ret)
+ return ret;
+
+ shift_sel = FIELD_GET(AN8855_SWITCH_EFUSE_R50O, val);
+ for (i = 0; i < ARRAY_SIZE(dsa_r50ohm_table); i++)
+ if (dsa_r50ohm_table[i] == shift_sel)
+ break;
+
+ if (i < 8 || i >= ARRAY_SIZE(dsa_r50ohm_table))
+ *dest = dsa_r50ohm_table[25];
+ else
+ *dest = dsa_r50ohm_table[i - 8];
+
+ return 0;
+}
+
+static int an8855_probe(struct phy_device *phydev)
+{
+ struct device *dev = &phydev->mdio.dev;
+ struct device_node *node = dev->of_node;
+ struct air_an8855_priv *priv;
+ int ret;
+
+ /* If we don't have a node, skip get calib */
+ if (!node)
+ return 0;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ ret = en8855_get_r50ohm_val(dev, "tx_a", &priv->calibration_data[0]);
+ if (ret)
+ return ret;
+
+ ret = en8855_get_r50ohm_val(dev, "tx_b", &priv->calibration_data[1]);
+ if (ret)
+ return ret;
+
+ ret = en8855_get_r50ohm_val(dev, "tx_c", &priv->calibration_data[2]);
+ if (ret)
+ return ret;
+
+ ret = en8855_get_r50ohm_val(dev, "tx_d", &priv->calibration_data[3]);
+ if (ret)
+ return ret;
+
+ phydev->priv = priv;
+
+ return 0;
+}
+
+static int an8855_get_downshift(struct phy_device *phydev, u8 *data)
+{
+ int saved_page;
+ int val;
+ int ret;
+
+ saved_page = phy_select_page(phydev, AN8855_PHY_PAGE_EXTENDED_1);
+ if (saved_page >= 0)
+ val = __phy_read(phydev, AN8855_PHY_EXT_REG_14);
+ ret = phy_restore_page(phydev, saved_page, val);
+ if (ret)
+ return ret;
+
+ *data = val & AN8855_PHY_EXT_REG_14 ? DOWNSHIFT_DEV_DEFAULT_COUNT :
+ DOWNSHIFT_DEV_DISABLE;
+
+ return 0;
+}
+
+static int an8855_set_downshift(struct phy_device *phydev, u8 cnt)
+{
+ int saved_page;
+ int ret;
+
+ saved_page = phy_select_page(phydev, AN8855_PHY_PAGE_EXTENDED_1);
+ if (saved_page >= 0) {
+ if (cnt != DOWNSHIFT_DEV_DISABLE)
+ ret = __phy_set_bits(phydev, AN8855_PHY_EXT_REG_14,
+ AN8855_PHY_EN_DOWN_SHFIT);
+ else
+ ret = __phy_clear_bits(phydev, AN8855_PHY_EXT_REG_14,
+ AN8855_PHY_EN_DOWN_SHFIT);
+ }
+
+ return phy_restore_page(phydev, saved_page, ret);
+}
+
+static int an8855_config_init(struct phy_device *phydev)
+{
+ struct air_an8855_priv *priv = phydev->priv;
+ int ret;
+
+ /* Enable HW auto downshift */
+ ret = an8855_set_downshift(phydev, DOWNSHIFT_DEV_DEFAULT_COUNT);
+ if (ret)
+ return ret;
+
+ /* Apply calibration values, if needed. BIT(0) signal this */
+ if (phydev->dev_flags & BIT(0)) {
+ u8 *calibration_data = priv->calibration_data;
+
+ ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, AN8855_PHY_R500HM_RSEL_TX_AB,
+ AN8855_PHY_R50OHM_RSEL_TX_A | AN8855_PHY_R50OHM_RSEL_TX_B,
+ FIELD_PREP(AN8855_PHY_R50OHM_RSEL_TX_A, calibration_data[0]) |
+ FIELD_PREP(AN8855_PHY_R50OHM_RSEL_TX_B, calibration_data[1]));
+ if (ret)
+ return ret;
+ ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, AN8855_PHY_R500HM_RSEL_TX_CD,
+ AN8855_PHY_R50OHM_RSEL_TX_C | AN8855_PHY_R50OHM_RSEL_TX_D,
+ FIELD_PREP(AN8855_PHY_R50OHM_RSEL_TX_C, calibration_data[2]) |
+ FIELD_PREP(AN8855_PHY_R50OHM_RSEL_TX_D, calibration_data[3]));
+ if (ret)
+ return ret;
+ }
+
+ /* Apply values to reduce signal noise */
+ ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, AN8855_PHY_TX_PAIR_DLY_SEL_GBE,
+ FIELD_PREP(AN8855_PHY_CR_DA_TX_PAIR_DELKAY_SEL_A_GBE, 0x4) |
+ FIELD_PREP(AN8855_PHY_CR_DA_TX_PAIR_DELKAY_SEL_C_GBE, 0x4));
+ if (ret)
+ return ret;
+ ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, AN8855_PHY_RXADC_CTRL,
+ AN8855_PHY_RG_AD_SAMNPLE_PHSEL_A |
+ AN8855_PHY_RG_AD_SAMNPLE_PHSEL_C);
+ if (ret)
+ return ret;
+ ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, AN8855_PHY_RXADC_REV_0,
+ FIELD_PREP(AN8855_PHY_RG_AD_RESERVE0_A, 0x1));
+ if (ret)
+ return ret;
+ ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, AN8855_PHY_RXADC_REV_1,
+ FIELD_PREP(AN8855_PHY_RG_AD_RESERVE0_C, 0x1));
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int an8855_get_tunable(struct phy_device *phydev,
+ struct ethtool_tunable *tuna, void *data)
+{
+ switch (tuna->id) {
+ case ETHTOOL_PHY_DOWNSHIFT:
+ return an8855_get_downshift(phydev, data);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int an8855_set_tunable(struct phy_device *phydev,
+ struct ethtool_tunable *tuna, const void *data)
+{
+ switch (tuna->id) {
+ case ETHTOOL_PHY_DOWNSHIFT:
+ return an8855_set_downshift(phydev, *(const u8 *)data);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int an8855_read_page(struct phy_device *phydev)
+{
+ return __phy_read(phydev, AN8855_PHY_SELECT_PAGE);
+}
+
+static int an8855_write_page(struct phy_device *phydev, int page)
+{
+ return __phy_write(phydev, AN8855_PHY_SELECT_PAGE, page);
+}
+
+static struct phy_driver an8855_driver[] = {
+{
+ PHY_ID_MATCH_EXACT(AN8855_PHY_ID),
+ .name = "Airoha AN8855 internal PHY",
+ /* PHY_GBIT_FEATURES */
+ .flags = PHY_IS_INTERNAL,
+ .probe = an8855_probe,
+ .config_init = an8855_config_init,
+ .soft_reset = genphy_soft_reset,
+ .get_tunable = an8855_get_tunable,
+ .set_tunable = an8855_set_tunable,
+ .suspend = genphy_suspend,
+ .resume = genphy_resume,
+ .read_page = an8855_read_page,
+ .write_page = an8855_write_page,
+}, };
+
+module_phy_driver(an8855_driver);
+
+static struct mdio_device_id __maybe_unused an8855_tbl[] = {
+ { PHY_ID_MATCH_EXACT(AN8855_PHY_ID) },
+ { }
+};
+
+MODULE_DEVICE_TABLE(mdio, an8855_tbl);
+
+MODULE_DESCRIPTION("Airoha AN8855 PHY driver");
+MODULE_AUTHOR("Christian Marangi <ansuelsmth@gmail.com>");
+MODULE_LICENSE("GPL");
--
2.45.2On Wed, Nov 06, 2024 at 01:22:38PM +0100, Christian Marangi wrote:
> +/* MII Registers Page 1 */
> +#define AN8855_PHY_EXT_REG_14 0x14
> +#define AN8855_PHY_EN_DOWN_SHFIT BIT(4)
Shouldn't "AN8855_PHY_EN_DOWN_SHFIT" be "AN8855_PHY_EN_DOWN_SHIFT"
(notice the I and F are swapped) ?
> +static int an8855_get_downshift(struct phy_device *phydev, u8 *data)
> +{
> + int saved_page;
> + int val;
> + int ret;
> +
> + saved_page = phy_select_page(phydev, AN8855_PHY_PAGE_EXTENDED_1);
> + if (saved_page >= 0)
> + val = __phy_read(phydev, AN8855_PHY_EXT_REG_14);
> + ret = phy_restore_page(phydev, saved_page, val);
> + if (ret)
> + return ret;
This function is entirely broken.
phy_restore_page() will return "val" if everything went successfully,
so here you end up returning "val" via this very return statement
without executing any further code in the function. The only time
further code will be executed is if "val" was successfully read as
zero.
Please use the helpers provided:
ret = phy_read_paged(phydev, AN8855_PHY_PAGE_EXTENDED_1,
AN8855_PHY_EXT_REG_14);
if (ret < 0)
return ret;
ret now contains what you're using as "val" below. No need to open code
phy_read_paged().
> +
> + *data = val & AN8855_PHY_EXT_REG_14 ? DOWNSHIFT_DEV_DEFAULT_COUNT :
> + DOWNSHIFT_DEV_DISABLE;
Here, the test is against the register number rather than the bit that
controls downshift. Shouldn't AN8855_PHY_EXT_REG_14 be
AN8855_PHY_EN_DOWN_SH(F)I(F)T ?
> +static int an8855_set_downshift(struct phy_device *phydev, u8 cnt)
> +{
> + int saved_page;
> + int ret;
> +
> + saved_page = phy_select_page(phydev, AN8855_PHY_PAGE_EXTENDED_1);
> + if (saved_page >= 0) {
> + if (cnt != DOWNSHIFT_DEV_DISABLE)
> + ret = __phy_set_bits(phydev, AN8855_PHY_EXT_REG_14,
> + AN8855_PHY_EN_DOWN_SHFIT);
> + else
> + ret = __phy_clear_bits(phydev, AN8855_PHY_EXT_REG_14,
> + AN8855_PHY_EN_DOWN_SHFIT);
> + }
> +
> + return phy_restore_page(phydev, saved_page, ret);
This entire thing can be simplified to:
u16 ds = cnt != DOWNSHIFT_DEV_DISABLE ? AN8855_PHY_EN_DOWN_SHFIT: 0;
return phy_modify_paged(phydev, AN8855_PHY_PAGE_EXTENDED_1,
AN8855_PHY_EXT_REG_14, AN8855_PHY_EN_DOWN_SHFIT,
ds);
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Fri, Nov 08, 2024 at 11:09:30AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 06, 2024 at 01:22:38PM +0100, Christian Marangi wrote:
> > +/* MII Registers Page 1 */
> > +#define AN8855_PHY_EXT_REG_14 0x14
> > +#define AN8855_PHY_EN_DOWN_SHFIT BIT(4)
>
> Shouldn't "AN8855_PHY_EN_DOWN_SHFIT" be "AN8855_PHY_EN_DOWN_SHIFT"
> (notice the I and F are swapped) ?
>
Typo from SDK that I didn't notice fun.
> > +static int an8855_get_downshift(struct phy_device *phydev, u8 *data)
> > +{
> > + int saved_page;
> > + int val;
> > + int ret;
> > +
> > + saved_page = phy_select_page(phydev, AN8855_PHY_PAGE_EXTENDED_1);
> > + if (saved_page >= 0)
> > + val = __phy_read(phydev, AN8855_PHY_EXT_REG_14);
> > + ret = phy_restore_page(phydev, saved_page, val);
> > + if (ret)
> > + return ret;
>
> This function is entirely broken.
>
> phy_restore_page() will return "val" if everything went successfully,
> so here you end up returning "val" via this very return statement
> without executing any further code in the function. The only time
> further code will be executed is if "val" was successfully read as
> zero.
>
> Please use the helpers provided:
>
> ret = phy_read_paged(phydev, AN8855_PHY_PAGE_EXTENDED_1,
> AN8855_PHY_EXT_REG_14);
> if (ret < 0)
> return ret;
>
> ret now contains what you're using as "val" below. No need to open code
> phy_read_paged().
Thanks for the explaination, totally got confused by reading the
restore_page code. Anyway yes I will use the helper.
>
> > +
> > + *data = val & AN8855_PHY_EXT_REG_14 ? DOWNSHIFT_DEV_DEFAULT_COUNT :
> > + DOWNSHIFT_DEV_DISABLE;
>
> Here, the test is against the register number rather than the bit that
> controls downshift. Shouldn't AN8855_PHY_EXT_REG_14 be
> AN8855_PHY_EN_DOWN_SH(F)I(F)T ?
Copy paste error, was already staged to fix, thanks for extra eye on
this.
>
> > +static int an8855_set_downshift(struct phy_device *phydev, u8 cnt)
> > +{
> > + int saved_page;
> > + int ret;
> > +
> > + saved_page = phy_select_page(phydev, AN8855_PHY_PAGE_EXTENDED_1);
> > + if (saved_page >= 0) {
> > + if (cnt != DOWNSHIFT_DEV_DISABLE)
> > + ret = __phy_set_bits(phydev, AN8855_PHY_EXT_REG_14,
> > + AN8855_PHY_EN_DOWN_SHFIT);
> > + else
> > + ret = __phy_clear_bits(phydev, AN8855_PHY_EXT_REG_14,
> > + AN8855_PHY_EN_DOWN_SHFIT);
> > + }
> > +
> > + return phy_restore_page(phydev, saved_page, ret);
>
> This entire thing can be simplified to:
>
> u16 ds = cnt != DOWNSHIFT_DEV_DISABLE ? AN8855_PHY_EN_DOWN_SHFIT: 0;
>
> return phy_modify_paged(phydev, AN8855_PHY_PAGE_EXTENDED_1,
> AN8855_PHY_EXT_REG_14, AN8855_PHY_EN_DOWN_SHFIT,
> ds);
Funnly in rechecking I produced the same exact change.
>
> Thanks.
Thanks to you for the review.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
--
Ansuel
Hi Christian,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/dt-bindings-net-dsa-Add-Airoha-AN8855-Gigabit-Switch-documentation/20241106-203624
base: net-next/main
patch link: https://lore.kernel.org/r/20241106122254.13228-4-ansuelsmth%40gmail.com
patch subject: [net-next PATCH v3 3/3] net: phy: Add Airoha AN8855 Internal Switch Gigabit PHY
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20241107/202411071000.uL10bu3r-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241107/202411071000.uL10bu3r-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411071000.uL10bu3r-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/net/phy/air_an8855.c:6:
In file included from include/linux/phy.h:16:
In file included from include/linux/ethtool.h:18:
In file included from include/linux/if_ether.h:19:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:8:
In file included from include/linux/cacheflush.h:5:
In file included from arch/x86/include/asm/cacheflush.h:5:
In file included from include/linux/mm.h:2213:
include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
504 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
505 | item];
| ~~~~
include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
511 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
512 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
524 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
525 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/phy/air_an8855.c:137:6: warning: variable 'val' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
137 | if (saved_page >= 0)
| ^~~~~~~~~~~~~~~
drivers/net/phy/air_an8855.c:139:45: note: uninitialized use occurs here
139 | ret = phy_restore_page(phydev, saved_page, val);
| ^~~
drivers/net/phy/air_an8855.c:137:2: note: remove the 'if' if its condition is always true
137 | if (saved_page >= 0)
| ^~~~~~~~~~~~~~~~~~~~
138 | val = __phy_read(phydev, AN8855_PHY_EXT_REG_14);
drivers/net/phy/air_an8855.c:133:9: note: initialize the variable 'val' to silence this warning
133 | int val;
| ^
| = 0
>> drivers/net/phy/air_an8855.c:155:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
155 | if (saved_page >= 0) {
| ^~~~~~~~~~~~~~~
drivers/net/phy/air_an8855.c:164:46: note: uninitialized use occurs here
164 | return phy_restore_page(phydev, saved_page, ret);
| ^~~
drivers/net/phy/air_an8855.c:155:2: note: remove the 'if' if its condition is always true
155 | if (saved_page >= 0) {
| ^~~~~~~~~~~~~~~~~~~~
drivers/net/phy/air_an8855.c:152:9: note: initialize the variable 'ret' to silence this warning
152 | int ret;
| ^
| = 0
6 warnings generated.
vim +137 drivers/net/phy/air_an8855.c
129
130 static int an8855_get_downshift(struct phy_device *phydev, u8 *data)
131 {
132 int saved_page;
133 int val;
134 int ret;
135
136 saved_page = phy_select_page(phydev, AN8855_PHY_PAGE_EXTENDED_1);
> 137 if (saved_page >= 0)
138 val = __phy_read(phydev, AN8855_PHY_EXT_REG_14);
139 ret = phy_restore_page(phydev, saved_page, val);
140 if (ret)
141 return ret;
142
143 *data = val & AN8855_PHY_EXT_REG_14 ? DOWNSHIFT_DEV_DEFAULT_COUNT :
144 DOWNSHIFT_DEV_DISABLE;
145
146 return 0;
147 }
148
149 static int an8855_set_downshift(struct phy_device *phydev, u8 cnt)
150 {
151 int saved_page;
152 int ret;
153
154 saved_page = phy_select_page(phydev, AN8855_PHY_PAGE_EXTENDED_1);
> 155 if (saved_page >= 0) {
156 if (cnt != DOWNSHIFT_DEV_DISABLE)
157 ret = __phy_set_bits(phydev, AN8855_PHY_EXT_REG_14,
158 AN8855_PHY_EN_DOWN_SHFIT);
159 else
160 ret = __phy_clear_bits(phydev, AN8855_PHY_EXT_REG_14,
161 AN8855_PHY_EN_DOWN_SHFIT);
162 }
163
164 return phy_restore_page(phydev, saved_page, ret);
165 }
166
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
> +static const u8 dsa_r50ohm_table[] = {
> + 127, 127, 127, 127, 127, 127, 127, 127, 127, 127,
> + 127, 127, 127, 127, 127, 127, 127, 126, 122, 117,
> + 112, 109, 104, 101, 97, 94, 90, 88, 84, 80,
> + 78, 74, 72, 68, 66, 64, 61, 58, 56, 53,
> + 51, 48, 47, 44, 42, 40, 38, 36, 34, 32,
> + 31, 28, 27, 24, 24, 22, 20, 18, 16, 16,
> + 14, 12, 11, 9
> +};
> +
> +static int en8855_get_r50ohm_val(struct device *dev, const char *calib_name,
> + u8 *dest)
> +{
> + u32 shift_sel, val;
> + int ret;
> + int i;
> +
> + ret = nvmem_cell_read_u32(dev, calib_name, &val);
> + if (ret)
> + return ret;
> +
> + shift_sel = FIELD_GET(AN8855_SWITCH_EFUSE_R50O, val);
> + for (i = 0; i < ARRAY_SIZE(dsa_r50ohm_table); i++)
> + if (dsa_r50ohm_table[i] == shift_sel)
> + break;
Is an exact match expected? Should this be >= so the nearest match is
found?
> +
> + if (i < 8 || i >= ARRAY_SIZE(dsa_r50ohm_table))
> + *dest = dsa_r50ohm_table[25];
> + else
> + *dest = dsa_r50ohm_table[i - 8];
> +
> + return 0;
> +}
> +
> +static int an8855_probe(struct phy_device *phydev)
> +{
> + struct device *dev = &phydev->mdio.dev;
> + struct device_node *node = dev->of_node;
> + struct air_an8855_priv *priv;
> + int ret;
> +
> + /* If we don't have a node, skip get calib */
> + if (!node)
> + return 0;
phydev->priv will be a NULL pointer, causing problems in
an8855_config_init()
Andrew
On Wed, Nov 06, 2024 at 05:19:03PM +0100, Andrew Lunn wrote:
> > +static const u8 dsa_r50ohm_table[] = {
> > + 127, 127, 127, 127, 127, 127, 127, 127, 127, 127,
> > + 127, 127, 127, 127, 127, 127, 127, 126, 122, 117,
> > + 112, 109, 104, 101, 97, 94, 90, 88, 84, 80,
> > + 78, 74, 72, 68, 66, 64, 61, 58, 56, 53,
> > + 51, 48, 47, 44, 42, 40, 38, 36, 34, 32,
> > + 31, 28, 27, 24, 24, 22, 20, 18, 16, 16,
> > + 14, 12, 11, 9
> > +};
> > +
> > +static int en8855_get_r50ohm_val(struct device *dev, const char *calib_name,
> > + u8 *dest)
> > +{
> > + u32 shift_sel, val;
> > + int ret;
> > + int i;
> > +
> > + ret = nvmem_cell_read_u32(dev, calib_name, &val);
> > + if (ret)
> > + return ret;
> > +
> > + shift_sel = FIELD_GET(AN8855_SWITCH_EFUSE_R50O, val);
> > + for (i = 0; i < ARRAY_SIZE(dsa_r50ohm_table); i++)
> > + if (dsa_r50ohm_table[i] == shift_sel)
> > + break;
>
> Is an exact match expected? Should this be >= so the nearest match is
> found?
>
As strange as this is, yes this is what the original code does.
> > +
> > + if (i < 8 || i >= ARRAY_SIZE(dsa_r50ohm_table))
> > + *dest = dsa_r50ohm_table[25];
> > + else
> > + *dest = dsa_r50ohm_table[i - 8];
> > +
> > + return 0;
> > +}
> > +
> > +static int an8855_probe(struct phy_device *phydev)
> > +{
> > + struct device *dev = &phydev->mdio.dev;
> > + struct device_node *node = dev->of_node;
> > + struct air_an8855_priv *priv;
> > + int ret;
> > +
> > + /* If we don't have a node, skip get calib */
> > + if (!node)
> > + return 0;
>
> phydev->priv will be a NULL pointer, causing problems in
> an8855_config_init()
>
Quite unlikely scenario since for the switch, defining the internal PHY
in an MDIO node is mandatory but yes it's a fragility.
2 solution:
- I check priv in config_init and skip that section
- I always set phydev->priv
Solution 1 is safer (handle case where for some reason
en8855_get_r50ohm_val fails (it's really almost impossible)) but error
prone if the PHY gets extended with other parts and priv starts to gets
used for other thing.
Solution 2 require an extra bool to signal full calibrarion read and is
waste more resource (in case calib is not needed...)
Anyway thanks for the review!
--
Ansuel
Hello Christian,
On Wed, 6 Nov 2024 13:22:38 +0100
Christian Marangi <ansuelsmth@gmail.com> wrote:
> Add support for Airoha AN8855 Internal Switch Gigabit PHY.
>
> This is a simple PHY driver to configure and calibrate the PHY for the
> AN8855 Switch with the use of NVMEM cells.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
[...]
> +static int an8855_get_downshift(struct phy_device *phydev, u8 *data)
> +{
> + int saved_page;
> + int val;
> + int ret;
> +
> + saved_page = phy_select_page(phydev, AN8855_PHY_PAGE_EXTENDED_1);
> + if (saved_page >= 0)
> + val = __phy_read(phydev, AN8855_PHY_EXT_REG_14);
> + ret = phy_restore_page(phydev, saved_page, val);
I think this can be replaced with phy_read_paged()
[...]
> +static int an8855_set_downshift(struct phy_device *phydev, u8 cnt)
> +{
> + int saved_page;
> + int ret;
> +
> + saved_page = phy_select_page(phydev, AN8855_PHY_PAGE_EXTENDED_1);
> + if (saved_page >= 0) {
> + if (cnt != DOWNSHIFT_DEV_DISABLE)
> + ret = __phy_set_bits(phydev, AN8855_PHY_EXT_REG_14,
> + AN8855_PHY_EN_DOWN_SHFIT);
> + else
> + ret = __phy_clear_bits(phydev, AN8855_PHY_EXT_REG_14,
> + AN8855_PHY_EN_DOWN_SHFIT);
> + }
> +
> + return phy_restore_page(phydev, saved_page, ret);
And this by phy_modify_paged() :)
Thanks,
Maxime
On Wed, Nov 06, 2024 at 03:54:58PM +0100, Maxime Chevallier wrote:
> Hello Christian,
>
> On Wed, 6 Nov 2024 13:22:38 +0100
> Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> > Add support for Airoha AN8855 Internal Switch Gigabit PHY.
> >
> > This is a simple PHY driver to configure and calibrate the PHY for the
> > AN8855 Switch with the use of NVMEM cells.
> >
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
>
> [...]
>
> > +static int an8855_get_downshift(struct phy_device *phydev, u8 *data)
> > +{
> > + int saved_page;
> > + int val;
> > + int ret;
> > +
> > + saved_page = phy_select_page(phydev, AN8855_PHY_PAGE_EXTENDED_1);
> > + if (saved_page >= 0)
> > + val = __phy_read(phydev, AN8855_PHY_EXT_REG_14);
> > + ret = phy_restore_page(phydev, saved_page, val);
>
> I think this can be replaced with phy_read_paged()
>
> [...]
>
> > +static int an8855_set_downshift(struct phy_device *phydev, u8 cnt)
> > +{
> > + int saved_page;
> > + int ret;
> > +
> > + saved_page = phy_select_page(phydev, AN8855_PHY_PAGE_EXTENDED_1);
> > + if (saved_page >= 0) {
> > + if (cnt != DOWNSHIFT_DEV_DISABLE)
> > + ret = __phy_set_bits(phydev, AN8855_PHY_EXT_REG_14,
> > + AN8855_PHY_EN_DOWN_SHFIT);
> > + else
> > + ret = __phy_clear_bits(phydev, AN8855_PHY_EXT_REG_14,
> > + AN8855_PHY_EN_DOWN_SHFIT);
> > + }
> > +
> > + return phy_restore_page(phydev, saved_page, ret);
>
> And this by phy_modify_paged() :)
>
Didn't notice those, even better! Thanks!
--
Ansuel
© 2016 - 2024 Red Hat, Inc.