[PATCH 1/2] phy: mxl-8611x: add driver for MaxLinear mxl-8611x PHYs
Nate Drude
nate.d at variscite.com
Thu Jul 13 02:14:18 CEST 2023
Hi Marek,
On 7/12/23 5:15 PM, Marek Vasut wrote:
> [You don't often get email from marek.vasut at mailbox.org. Learn why this
> is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> On 7/12/23 22:48, Nate Drude wrote:
>> The MxL86110 is a low power Ethernet PHY transceiver integrated
>> circuit following the IEEE 802.3 [1] standard. It offers a
>> cost-optimized solution that is well-suited for routers,
>> switches, and home gateways. It performs data transmission on
>> an Ethernet twisted pair copper cable of category CAT5e or higher.
>> The MxL86110 supports 1000, 100, and 10 Mbit/s data rates.
>>
>> The current driver implementation supports:
>> - configuring rgmii from the device tree
>> - configuring the LEDs from the device tree
>> - reading extended registers using the mdio command, e.g.:
>> => mdio rx ethernet at 428a0000 0xa00c
>> Reading from bus ethernet at 428a0000
>> PHY at address 0:
>> 40972 - 0x2600
>>
>> Signed-off-by: Nate Drude <nate.d at variscite.com>
>> ---
>> drivers/net/phy/Kconfig | 5 +
>> drivers/net/phy/Makefile | 1 +
>> drivers/net/phy/mxl-8611x.c | 271 ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 277 insertions(+)
>> create mode 100644 drivers/net/phy/mxl-8611x.c
>>
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index 24158776f52..0fc0fb2c5bf 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -220,6 +220,11 @@ config PHY_MICREL_KSZ8XXX
>>
>> endif # PHY_MICREL
>>
>> +config PHY_MXL8611X
>> + bool "MaxLinear MXL8611X Ethernet PHYs"
>> + help
>> + Add support for configuring MaxLinear MXL8611X Ethernet PHYs.
>
> Probably better just "Support for MaxLinear MXL8611X Ethernet PHYs." in
> the help text.
>
>> config PHY_MSCC
>> bool "Microsemi Corp Ethernet PHYs support"
>>
>> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>> index 85d17f109cd..b3f4d75936c 100644
>> --- a/drivers/net/phy/Makefile
>> +++ b/drivers/net/phy/Makefile
>> @@ -24,6 +24,7 @@ obj-$(CONFIG_PHY_MARVELL_10G) += marvell10g.o
>> obj-$(CONFIG_PHY_MICREL_KSZ8XXX) += micrel_ksz8xxx.o
>> obj-$(CONFIG_PHY_MICREL_KSZ90X1) += micrel_ksz90x1.o
>> obj-$(CONFIG_PHY_MESON_GXL) += meson-gxl.o
>> +obj-$(CONFIG_PHY_MXL8611X) += mxl-8611x.o
>> obj-$(CONFIG_PHY_NATSEMI) += natsemi.o
>> obj-$(CONFIG_PHY_NXP_C45_TJA11XX) += nxp-c45-tja11xx.o
>> obj-$(CONFIG_PHY_NXP_TJA11XX) += nxp-tja11xx.o
>> diff --git a/drivers/net/phy/mxl-8611x.c b/drivers/net/phy/mxl-8611x.c
>> new file mode 100644
>> index 00000000000..467edc5bb5f
>> --- /dev/null
>> +++ b/drivers/net/phy/mxl-8611x.c
>> @@ -0,0 +1,271 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/**
>> + * Driver for MaxLinear MXL861100 Ethernet PHY
>
> 611x ... you have either 1 or 0 duplicated above.
>
>> + * Copyright 2023 Variscite Ltd.
>> + * Copyright 2023 MaxLinear Inc.
>> + * Author: Nate Drude <nate.d at variscite.com>
>> + */
>> +#include <common.h>
>> +#include <phy.h>
>> +#include <linux/bitops.h>
>> +#include <linux/bitfield.h>
>> +
>> +/* PHY IDs */
>> +#define PHY_ID_MXL86110 0xC1335580
>> +#define PHY_ID_MXL86111 0xC1335588
>> +
>> +/* required to access extended registers */
>> +#define MXL8611X_EXTD_REG_ADDR_OFFSET 0x1E
>> +#define MXL8611X_EXTD_REG_ADDR_DATA 0x1F
>> +
>> +/* RGMII register */
>> +#define MXL8611X_EXT_RGMII_CFG1_REG 0xA003
>> +#define MXL8611X_EXT_RGMII_CFG1_NO_DELAY 0
>> +
>> +#define MXL8611X_EXT_RGMII_CFG1_RX_DELAY_MASK GENMASK(13, 10)
>> +#define MXL8611X_EXT_RGMII_CFG1_TX_1G_DELAY_MASK GENMASK(3, 0)
>> +#define MXL8611X_EXT_RGMII_CFG1_TX_10MB_100MB_DELAY_MASK
>> GENMASK(7, 4)
>> +
>> +/* LED registers and defines */
>> +#define MXL8611X_LED0_CFG_REG 0xA00C
>> +#define MXL8611X_LED1_CFG_REG 0xA00D
>> +#define MXL8611X_LED2_CFG_REG 0xA00E
>> +
>> +/**
>> + * struct mxl8611x_cfg_reg_map - map a config value to aregister value
>> + * @cfg value in device configuration
>> + * @reg value in the register
>> + */
>> +struct mxl8611x_cfg_reg_map {
>> + int cfg;
>> + int reg;
>> +};
>> +
>> +static const struct mxl8611x_cfg_reg_map mxl8611x_rgmii_delays[] = {
>> + { 0, 0 },
>> + { 150, 1 },
>
> Is this like n * 150 expanded into a table here ?
> Please drop.
>
>> + { 300, 2 },
>> + { 450, 3 },
>> + { 600, 4 },
>> + { 750, 5 },
>> + { 900, 6 },
>> + { 1050, 7 },
>> + { 1200, 8 },
>> + { 1350, 9 },
>> + { 1500, 10 },
>> + { 1650, 11 },
>> + { 1800, 12 },
>> + { 1950, 13 },
>> + { 2100, 14 },
>> + { 2250, 15 },
>> + { 0, 0 } // Marks the end of the array
>> +};
>> +
>> +static int mxl8611x_lookup_reg_value(const struct mxl8611x_cfg_reg_map
>> *tbl,
>> + const int cfg, int *reg)
>> +{
>> + size_t i;
>> +
>> + for (i = 0; i == 0 || tbl[i].cfg; i++) {
>> + if (tbl[i].cfg == cfg) {
>> + *reg = tbl[i].reg;
>> + return 0;
>> + }
>> + }
>
> value / 150 instead of this ?
>
>> + return -EINVAL;
>> +}
>> +
>> +static u16 mxl8611x_ext_read(struct phy_device *phydev, const u32
>> regnum)
>> +{
>> + u16 val;
>> +
>> + phy_write(phydev, MDIO_DEVAD_NONE, MXL8611X_EXTD_REG_ADDR_OFFSET,
>> regnum);
>> + val = phy_read(phydev, MDIO_DEVAD_NONE,
>> MXL8611X_EXTD_REG_ADDR_DATA);
>
> Is this some phy_read_mmd() reimplementation here ?
>
>> + debug("%s: MXL86110 at 0x%x 0x%x=0x%x\n", __func__, phydev->addr,
>> regnum, val);
>> +
>> + return val;
>> +}
>> +
>> +static int mxl8611x_ext_write(struct phy_device *phydev, const u32
>> regnum, const u16 val)
>> +{
>> + debug("%s: MXL86110 at 0x%x 0x%x=0x%x\n", __func__, phydev->addr,
>> regnum, val);
>> +
>> + phy_write(phydev, MDIO_DEVAD_NONE, MXL8611X_EXTD_REG_ADDR_OFFSET,
>> regnum);
>> +
>> + return phy_write(phydev, MDIO_DEVAD_NONE,
>> MXL8611X_EXTD_REG_ADDR_DATA, val);
>> +}
>> +
>> +static int mxl8611x_extread(struct phy_device *phydev, int addr, int
>> devaddr,
>> + int regnum)
>> +{
>> + return mxl8611x_ext_read(phydev, regnum);
>> +}
>> +
>> +static int mxl8611x_extwrite(struct phy_device *phydev, int addr,
>> + int devaddr, int regnum, u16 val)
>> +{
>> + return mxl8611x_ext_write(phydev, regnum, val);
>> +}
>> +
>> +static int mxl8611x_led_cfg(struct phy_device *phydev)
>> +{
>> + int ret = 0;
>
> No need to init ret to 0 here, it is inited in snprintf below.
>
>> + int i;
>> + char propname[25];
>> + u32 val;
>> +
>> + ofnode node = phy_get_ofnode(phydev);
>> +
>> + if (!ofnode_valid(node)) {
>> + printf("%s: failed to get node\n", __func__);
>
> Try 'dev_err(phydev->dev,' instead of the printf()s
>
>> + return -EINVAL;
>> + }
>> +
>> + /* Loop through three the LED registers */
>> + for (i = 0; i < 3; i++) {
>> + /* Read property from device tree */
>> + ret = snprintf(propname, 25, "mxl-8611x,led%d_cfg", i);
>
> Instead of hardcoding 25, use 'sizeof(propname)' .
>
>> + if (ofnode_read_u32(node, propname, &val))
>> + continue;
>
> It might be simpler to call this thrice instead of a loop here:
>
> ofnode_read_u32_default();
> ...ext_write();
>
> And just initialize the registers with default values of the prop is
> missing in DT. And you avoid the snprintf() too.
>
>> + /* Update PHY LED register */
>> + mxl8611x_ext_write(phydev, MXL8611X_LED0_CFG_REG + i, val);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int mxl8611x_rgmii_cfg_of_delay(struct phy_device *phydev, const
>> char *property, int *val)
>> +{
>> + u32 of_val;
>> + int ret;
>> +
>> + ofnode node = phy_get_ofnode(phydev);
>> +
>> + if (!ofnode_valid(node)) {
>> + printf("%s: failed to get node\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + /* Get property from dts */
>> + ret = ofnode_read_u32(node, property, &of_val);
>> + if (ret)
>> + return ret;
>> +
>> + /* Convert delay in ps to register value */
>> + ret = mxl8611x_lookup_reg_value(mxl8611x_rgmii_delays, of_val, val);
>> + if (ret)
>> + printf("%s: Error: %s = %d is invalid, using default value\n",
>> + __func__, property, of_val);
>> +
>> + return ret;
>> +}
>> +
>> +static int mxl8611x_rgmii_cfg(struct phy_device *phydev)
>> +{
>> + u32 val = 0;
>> + int rxdelay, txdelay_100m, txdelay_1g;
>> +
>> + /* Get rgmii register value */
>> + val = mxl8611x_ext_read(phydev, MXL8611X_EXT_RGMII_CFG1_REG);
>> +
>> + /* Get RGMII Rx Delay Selection from device tree or rgmii
>> register */
>> + if (mxl8611x_rgmii_cfg_of_delay(phydev,
>> "mxl-8611x,rx-internal-delay-ps", &rxdelay))
>> + rxdelay = FIELD_GET(MXL8611X_EXT_RGMII_CFG1_RX_DELAY_MASK, val);
>> +
>> + /* Get Fast Ethernet RGMII Tx Clock Delay Selection from device
>> tree or rgmii register */
>> + if (mxl8611x_rgmii_cfg_of_delay(phydev,
>
> Use this form please, fix globally:
>
> ret = operation();
> if (ret)
> handle_error();
>
> or
>
> ret = operation();
> if (ret)
> goto err_handler;
>
>> "mxl-8611x,tx-internal-delay-ps-100m",
>> + &txdelay_100m))
>> + txdelay_100m =
>> FIELD_GET(MXL8611X_EXT_RGMII_CFG1_TX_10MB_100MB_DELAY_MASK, val);
>> +
>> + /* Get Gigabit Ethernet RGMII Tx Clock Delay Selection from device
>> tree or rgmii register */
>> + if (mxl8611x_rgmii_cfg_of_delay(phydev,
>> "mxl-8611x,tx-internal-delay-ps-1g", &txdelay_1g))
>> + txdelay_1g =
>> FIELD_GET(MXL8611X_EXT_RGMII_CFG1_TX_1G_DELAY_MASK, val);
>> +
>> + switch (phydev->interface) {
>> + case PHY_INTERFACE_MODE_RGMII:
>> + val = MXL8611X_EXT_RGMII_CFG1_NO_DELAY;
>> + break;
>> + case PHY_INTERFACE_MODE_RGMII_RXID:
>> + val = (val & ~MXL8611X_EXT_RGMII_CFG1_RX_DELAY_MASK) |
>> + FIELD_PREP(MXL8611X_EXT_RGMII_CFG1_RX_DELAY_MASK, rxdelay);
>> + break;
>> + case PHY_INTERFACE_MODE_RGMII_TXID:
>> + val = (val &
>> ~MXL8611X_EXT_RGMII_CFG1_TX_10MB_100MB_DELAY_MASK) |
>> +
>> FIELD_PREP(MXL8611X_EXT_RGMII_CFG1_TX_10MB_100MB_DELAY_MASK,
>> txdelay_100m);
>> + val = (val & ~MXL8611X_EXT_RGMII_CFG1_TX_1G_DELAY_MASK) |
>> + FIELD_PREP(MXL8611X_EXT_RGMII_CFG1_TX_1G_DELAY_MASK,
>> txdelay_1g);
>> + break;
>> + case PHY_INTERFACE_MODE_RGMII_ID:
>> + val = (val & ~MXL8611X_EXT_RGMII_CFG1_RX_DELAY_MASK) |
>> + FIELD_PREP(MXL8611X_EXT_RGMII_CFG1_RX_DELAY_MASK, rxdelay);
>> + val = (val &
>> ~MXL8611X_EXT_RGMII_CFG1_TX_10MB_100MB_DELAY_MASK) |
>> +
>> FIELD_PREP(MXL8611X_EXT_RGMII_CFG1_TX_10MB_100MB_DELAY_MASK,
>> txdelay_100m);
>> + val = (val & ~MXL8611X_EXT_RGMII_CFG1_TX_1G_DELAY_MASK) |
>> + FIELD_PREP(MXL8611X_EXT_RGMII_CFG1_TX_1G_DELAY_MASK,
>> txdelay_1g);
>> + break;
>> + default:
>> + printf("%s: Error: Unsupported rgmii mode %d\n", __func__,
>
> dev_err()
>
>> phydev->interface);
>> + return -EINVAL;
>> + }
>> +
>> + return mxl8611x_ext_write(phydev, MXL8611X_EXT_RGMII_CFG1_REG, val);
>> +}
>> +
>> +static int mxl8611x_config(struct phy_device *phydev)
>> +{
>> + int ret;
>> +
>> + /* Configure rgmii */
>> + ret = mxl8611x_rgmii_cfg(phydev);
>> +
>
> Drop newline
>
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* Configure LEDs */
>> + ret = mxl8611x_led_cfg(phydev);
>> +
>
> Drop newline
>
>> + if (ret < 0)
>> + return ret;
>> +
>> + return genphy_config(phydev);
>> +}
>> +
>> +static int mxl86110_config(struct phy_device *phydev)
>> +{
>> + printf("MXL86110 PHY detected at addr %d\n", phydev->addr);
>
> Drop the printf
>
>> + return mxl8611x_config(phydev);
>> +}
>> +
>> +static int mxl86111_config(struct phy_device *phydev)
>> +{
>> + printf("MXL86111 PHY detected at addr %d\n", phydev->addr);
>
> Drop the printf
>
>> + return mxl8611x_config(phydev);
>> +}
>> +
>> +U_BOOT_PHY_DRIVER(mxl86110) = {
>> + .name = "MXL86110",
>> + .uid = PHY_ID_MXL86110,
>> + .mask = 0xffffffff,
>> + .features = PHY_GBIT_FEATURES,
>> + .config = mxl86110_config,
>
> This can be unified into single PHY driver , see printf above, just
> tweak the .mask accordingly.
>
>> + .startup = genphy_startup,
>> + .shutdown = genphy_shutdown,
>> + .readext = mxl8611x_extread,
>> + .writeext = mxl8611x_extwrite,
>> +};
>> +
>> +U_BOOT_PHY_DRIVER(mxl86111) = {
>> + .name = "MXL86111",
>> + .uid = PHY_ID_MXL86111,
>> + .mask = 0xffffffff,
>> + .features = PHY_GBIT_FEATURES,
>> + .config = mxl86111_config,
>> + .startup = genphy_startup,
>> + .shutdown = genphy_shutdown,
>> + .readext = mxl8611x_extread,
>> + .writeext = mxl8611x_extwrite,
>> +};
>
> --
> Best regards,
> Marek Vasut
>
Thanks for your review. I will make these changes and send a v2 of the
patches.
Sincerely,
Nate
More information about the U-Boot
mailing list