| Message ID | 20250218013653.229234-2-kylehendrydev@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: bcm63xx: Enable internal GPHY on BCM63268 | expand |
On Mon, Feb 17, 2025 at 05:36:40PM -0800, Kyle Hendry wrote: > This patch adds support for the internal gigabit PHY on the Please do not use "This commit/patch/change", but imperative mood. See longer explanation here: https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > BCM63268 SoC. The PHY has a low power mode that has can be > enabled/disabled through the GPHY control register. The > register is passed in through the device tree, and the > relevant bits are set in the suspend and resume functions. > ... > +int bcm63268_gphy_resume(struct phy_device *phydev) > +{ > + int ret; > + > + ret = bcm63268_gphy_set(phydev, true); > + if (ret) > + return ret; > + > + ret = genphy_resume(phydev); > + if (ret) > + return ret; > + > + return 0; > +} > + > +int bcm63268_gphy_suspend(struct phy_device *phydev) Why these are not static? Where is EXPORT_SYMBOL and kerneldoc? > +{ > + int ret; > + > + ret = genphy_suspend(phydev); > + if (ret) > + return ret; > + > + ret = bcm63268_gphy_set(phydev, false); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int bcm63268_gphy_probe(struct phy_device *phydev) > +{ > + struct device_node *np = dev_of_node(&phydev->mdio.bus->dev); > + struct mdio_device *mdio = &phydev->mdio; > + struct device *dev = &mdio->dev; > + struct bcm_gphy_priv *priv; > + struct regmap *regmap; > + int err; > + > + err = devm_phy_package_join(dev, phydev, 0, 0); > + if (err) > + return err; > + > + priv = devm_kzalloc(dev, sizeof(struct bcm_gphy_priv), GFP_KERNEL); sizeof(*) > + if (!priv) > + return -ENOMEM; > + > + phydev->priv = priv; > + > + regmap = syscon_regmap_lookup_by_phandle(np, "brcm,gphy-ctrl"); No. ABI break without any explanation in commit msg. Best regards, Krzysztof
On 18/02/2025 02:36, Kyle Hendry wrote: > This patch adds support for the internal gigabit PHY on the > BCM63268 SoC. The PHY has a low power mode that has can be > enabled/disabled through the GPHY control register. The > register is passed in through the device tree, and the > relevant bits are set in the suspend and resume functions. > > Signed-off-by: Kyle Hendry <kylehendrydev@gmail.com> > --- > drivers/net/phy/bcm63xx.c | 96 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 96 insertions(+) Also two more nits: > > diff --git a/drivers/net/phy/bcm63xx.c b/drivers/net/phy/bcm63xx.c > index b46a736a3130..613c3da315ac 100644 > --- a/drivers/net/phy/bcm63xx.c > +++ b/drivers/net/phy/bcm63xx.c > @@ -3,8 +3,11 @@ > * Driver for Broadcom 63xx SOCs integrated PHYs > */ > #include "bcm-phy-lib.h" > +#include <linux/mfd/syscon.h> > #include <linux/module.h> > #include <linux/phy.h> > +#include <linux/regmap.h> > + > No need for new line. > #define MII_BCM63XX_IR 0x1a /* interrupt register */ > #define MII_BCM63XX_IR_EN 0x4000 /* global interrupt enable */ > @@ -13,10 +16,19 @@ > #define MII_BCM63XX_IR_LINK 0x0200 /* link changed */ > #define MII_BCM63XX_IR_GMASK 0x0100 /* global interrupt mask */ > > +#define PHY_ID_BCM63268_GPHY 0x03625f50 > + > +#define GPHY_CTRL_IDDQ_BIAS BIT(0) > +#define GPHY_CTRL_LOW_PWR BIT(3) > + > MODULE_DESCRIPTION("Broadcom 63xx internal PHY driver"); > MODULE_AUTHOR("Maxime Bizon <mbizon@freebox.fr>"); > MODULE_LICENSE("GPL"); > > +struct bcm_gphy_priv { > + struct regmap *gphy_ctrl; Messed indentation. > +}; > + > static int bcm63xx_config_intr(struct phy_device *phydev) > { > int reg, err; > @@ -69,6 +81,80 @@ static int bcm63xx_config_init(struct phy_device *phydev) > return phy_write(phydev, MII_BCM63XX_IR, reg); > } Best regards, Krzysztof
Hi Kyle, kernel test robot noticed the following build warnings: [auto build test WARNING on lee-mfd/for-mfd-next] [also build test WARNING on robh/for-next lee-leds/for-leds-next linus/master lee-mfd/for-mfd-fixes v6.14-rc3 next-20250218] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Kyle-Hendry/net-phy-bcm63xx-add-support-for-BCM63268-GPHY/20250218-094117 base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next patch link: https://lore.kernel.org/r/20250218013653.229234-2-kylehendrydev%40gmail.com patch subject: [PATCH v2 1/5] net: phy: bcm63xx: add support for BCM63268 GPHY config: x86_64-buildonly-randconfig-004-20250219 (https://download.01.org/0day-ci/archive/20250219/202502191212.s0NqhQ3T-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/20250219/202502191212.s0NqhQ3T-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/202502191212.s0NqhQ3T-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/net/phy/bcm63xx.c:84:5: warning: no previous prototype for function 'bcm63268_gphy_set' [-Wmissing-prototypes] 84 | int bcm63268_gphy_set(struct phy_device *phydev, bool enable) | ^ drivers/net/phy/bcm63xx.c:84:1: note: declare 'static' if the function is not intended to be used outside of this translation unit 84 | int bcm63268_gphy_set(struct phy_device *phydev, bool enable) | ^ | static >> drivers/net/phy/bcm63xx.c:100:5: warning: no previous prototype for function 'bcm63268_gphy_resume' [-Wmissing-prototypes] 100 | int bcm63268_gphy_resume(struct phy_device *phydev) | ^ drivers/net/phy/bcm63xx.c:100:1: note: declare 'static' if the function is not intended to be used outside of this translation unit 100 | int bcm63268_gphy_resume(struct phy_device *phydev) | ^ | static >> drivers/net/phy/bcm63xx.c:115:5: warning: no previous prototype for function 'bcm63268_gphy_suspend' [-Wmissing-prototypes] 115 | int bcm63268_gphy_suspend(struct phy_device *phydev) | ^ drivers/net/phy/bcm63xx.c:115:1: note: declare 'static' if the function is not intended to be used outside of this translation unit 115 | int bcm63268_gphy_suspend(struct phy_device *phydev) | ^ | static 3 warnings generated. vim +/bcm63268_gphy_set +84 drivers/net/phy/bcm63xx.c 83 > 84 int bcm63268_gphy_set(struct phy_device *phydev, bool enable) 85 { 86 struct bcm_gphy_priv *priv = phydev->priv; 87 u32 pwr_bits; 88 int ret; 89 90 pwr_bits = GPHY_CTRL_IDDQ_BIAS | GPHY_CTRL_LOW_PWR; 91 92 if (enable) 93 ret = regmap_update_bits(priv->gphy_ctrl, 0, pwr_bits, 0); 94 else 95 ret = regmap_update_bits(priv->gphy_ctrl, 0, pwr_bits, pwr_bits); 96 97 return ret; 98 } 99 > 100 int bcm63268_gphy_resume(struct phy_device *phydev) 101 { 102 int ret; 103 104 ret = bcm63268_gphy_set(phydev, true); 105 if (ret) 106 return ret; 107 108 ret = genphy_resume(phydev); 109 if (ret) 110 return ret; 111 112 return 0; 113 } 114 > 115 int bcm63268_gphy_suspend(struct phy_device *phydev) 116 { 117 int ret; 118 119 ret = genphy_suspend(phydev); 120 if (ret) 121 return ret; 122 123 ret = bcm63268_gphy_set(phydev, false); 124 if (ret) 125 return ret; 126 127 return 0; 128 } 129
Hi Kyle, kernel test robot noticed the following build warnings: [auto build test WARNING on lee-mfd/for-mfd-next] [also build test WARNING on robh/for-next lee-leds/for-leds-next linus/master lee-mfd/for-mfd-fixes v6.14-rc3 next-20250218] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Kyle-Hendry/net-phy-bcm63xx-add-support-for-BCM63268-GPHY/20250218-094117 base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next patch link: https://lore.kernel.org/r/20250218013653.229234-2-kylehendrydev%40gmail.com patch subject: [PATCH v2 1/5] net: phy: bcm63xx: add support for BCM63268 GPHY config: loongarch-randconfig-001-20250219 (https://download.01.org/0day-ci/archive/20250219/202502191246.zER47JXl-lkp@intel.com/config) compiler: loongarch64-linux-gcc (GCC) 14.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250219/202502191246.zER47JXl-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/202502191246.zER47JXl-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/net/phy/bcm63xx.c:84:5: warning: no previous prototype for 'bcm63268_gphy_set' [-Wmissing-prototypes] 84 | int bcm63268_gphy_set(struct phy_device *phydev, bool enable) | ^~~~~~~~~~~~~~~~~ >> drivers/net/phy/bcm63xx.c:100:5: warning: no previous prototype for 'bcm63268_gphy_resume' [-Wmissing-prototypes] 100 | int bcm63268_gphy_resume(struct phy_device *phydev) | ^~~~~~~~~~~~~~~~~~~~ >> drivers/net/phy/bcm63xx.c:115:5: warning: no previous prototype for 'bcm63268_gphy_suspend' [-Wmissing-prototypes] 115 | int bcm63268_gphy_suspend(struct phy_device *phydev) | ^~~~~~~~~~~~~~~~~~~~~ vim +/bcm63268_gphy_set +84 drivers/net/phy/bcm63xx.c 83 > 84 int bcm63268_gphy_set(struct phy_device *phydev, bool enable) 85 { 86 struct bcm_gphy_priv *priv = phydev->priv; 87 u32 pwr_bits; 88 int ret; 89 90 pwr_bits = GPHY_CTRL_IDDQ_BIAS | GPHY_CTRL_LOW_PWR; 91 92 if (enable) 93 ret = regmap_update_bits(priv->gphy_ctrl, 0, pwr_bits, 0); 94 else 95 ret = regmap_update_bits(priv->gphy_ctrl, 0, pwr_bits, pwr_bits); 96 97 return ret; 98 } 99 > 100 int bcm63268_gphy_resume(struct phy_device *phydev) 101 { 102 int ret; 103 104 ret = bcm63268_gphy_set(phydev, true); 105 if (ret) 106 return ret; 107 108 ret = genphy_resume(phydev); 109 if (ret) 110 return ret; 111 112 return 0; 113 } 114 > 115 int bcm63268_gphy_suspend(struct phy_device *phydev) 116 { 117 int ret; 118 119 ret = genphy_suspend(phydev); 120 if (ret) 121 return ret; 122 123 ret = bcm63268_gphy_set(phydev, false); 124 if (ret) 125 return ret; 126 127 return 0; 128 } 129
diff --git a/drivers/net/phy/bcm63xx.c b/drivers/net/phy/bcm63xx.c index b46a736a3130..613c3da315ac 100644 --- a/drivers/net/phy/bcm63xx.c +++ b/drivers/net/phy/bcm63xx.c @@ -3,8 +3,11 @@ * Driver for Broadcom 63xx SOCs integrated PHYs */ #include "bcm-phy-lib.h" +#include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/phy.h> +#include <linux/regmap.h> + #define MII_BCM63XX_IR 0x1a /* interrupt register */ #define MII_BCM63XX_IR_EN 0x4000 /* global interrupt enable */ @@ -13,10 +16,19 @@ #define MII_BCM63XX_IR_LINK 0x0200 /* link changed */ #define MII_BCM63XX_IR_GMASK 0x0100 /* global interrupt mask */ +#define PHY_ID_BCM63268_GPHY 0x03625f50 + +#define GPHY_CTRL_IDDQ_BIAS BIT(0) +#define GPHY_CTRL_LOW_PWR BIT(3) + MODULE_DESCRIPTION("Broadcom 63xx internal PHY driver"); MODULE_AUTHOR("Maxime Bizon <mbizon@freebox.fr>"); MODULE_LICENSE("GPL"); +struct bcm_gphy_priv { + struct regmap *gphy_ctrl; +}; + static int bcm63xx_config_intr(struct phy_device *phydev) { int reg, err; @@ -69,6 +81,80 @@ static int bcm63xx_config_init(struct phy_device *phydev) return phy_write(phydev, MII_BCM63XX_IR, reg); } +int bcm63268_gphy_set(struct phy_device *phydev, bool enable) +{ + struct bcm_gphy_priv *priv = phydev->priv; + u32 pwr_bits; + int ret; + + pwr_bits = GPHY_CTRL_IDDQ_BIAS | GPHY_CTRL_LOW_PWR; + + if (enable) + ret = regmap_update_bits(priv->gphy_ctrl, 0, pwr_bits, 0); + else + ret = regmap_update_bits(priv->gphy_ctrl, 0, pwr_bits, pwr_bits); + + return ret; +} + +int bcm63268_gphy_resume(struct phy_device *phydev) +{ + int ret; + + ret = bcm63268_gphy_set(phydev, true); + if (ret) + return ret; + + ret = genphy_resume(phydev); + if (ret) + return ret; + + return 0; +} + +int bcm63268_gphy_suspend(struct phy_device *phydev) +{ + int ret; + + ret = genphy_suspend(phydev); + if (ret) + return ret; + + ret = bcm63268_gphy_set(phydev, false); + if (ret) + return ret; + + return 0; +} + +static int bcm63268_gphy_probe(struct phy_device *phydev) +{ + struct device_node *np = dev_of_node(&phydev->mdio.bus->dev); + struct mdio_device *mdio = &phydev->mdio; + struct device *dev = &mdio->dev; + struct bcm_gphy_priv *priv; + struct regmap *regmap; + int err; + + err = devm_phy_package_join(dev, phydev, 0, 0); + if (err) + return err; + + priv = devm_kzalloc(dev, sizeof(struct bcm_gphy_priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + phydev->priv = priv; + + regmap = syscon_regmap_lookup_by_phandle(np, "brcm,gphy-ctrl"); + if (IS_ERR(regmap)) + return PTR_ERR(regmap); + + priv->gphy_ctrl = regmap; + + return 0; +} + static struct phy_driver bcm63xx_driver[] = { { .phy_id = 0x00406000, @@ -89,6 +175,15 @@ static struct phy_driver bcm63xx_driver[] = { .config_init = bcm63xx_config_init, .config_intr = bcm63xx_config_intr, .handle_interrupt = bcm_phy_handle_interrupt, +}, { + .phy_id = PHY_ID_BCM63268_GPHY, + .phy_id_mask = 0xfffffff0, + .name = "Broadcom BCM63268 GPHY", + /* PHY_BASIC_FEATURES */ + .flags = PHY_IS_INTERNAL, + .probe = bcm63268_gphy_probe, + .resume = bcm63268_gphy_resume, + .suspend = bcm63268_gphy_suspend, } }; module_phy_driver(bcm63xx_driver); @@ -96,6 +191,7 @@ module_phy_driver(bcm63xx_driver); static const struct mdio_device_id __maybe_unused bcm63xx_tbl[] = { { 0x00406000, 0xfffffc00 }, { 0x002bdc00, 0xfffffc00 }, + { PHY_ID_MATCH_EXACT(PHY_ID_BCM63268_GPHY) }, { } };
This patch adds support for the internal gigabit PHY on the BCM63268 SoC. The PHY has a low power mode that has can be enabled/disabled through the GPHY control register. The register is passed in through the device tree, and the relevant bits are set in the suspend and resume functions. Signed-off-by: Kyle Hendry <kylehendrydev@gmail.com> --- drivers/net/phy/bcm63xx.c | 96 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+)