| Message ID | 20240201151747.7524-1-ansuelsmth@gmail.com |
|---|---|
| Headers | show |
| Series | net: phy: Introduce PHY Package concept | expand |
On Fri, Feb 02, 2024 at 08:41:56AM +0100, Krzysztof Kozlowski wrote: > On 01/02/2024 16:17, Christian Marangi wrote: > > From: Robert Marko <robert.marko@sartura.hr> > > > > Add DT bindings defined for Qualcomm QCA807x PHY series related to > > calibration and DAC settings. > > Nothing from this file is used and your commit msg does not provide > rationale "why", thus it does not look like something for bindings. > Otherwise please point me which patch with *driver* uses these bindings. > Hi, since I have to squash this, I will include the reason in the schema patch. Anyway these are raw values used to configure the qcom,control-dac property. In the driver it's used by qca807x_config_init. We read what is set in DT and we configure the reg accordingly. If this is wrong should we use a more schema friendly approach with declaring an enum of string and document that there? > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > --- > > include/dt-bindings/net/qcom-qca807x.h | 30 ++++++++++++++++++++++++++ > > Use filename matching compatible, so vendor,device. No wildcards, unless > your compatible also has them. > > > 1 file changed, 30 insertions(+) > > create mode 100644 include/dt-bindings/net/qcom-qca807x.h > > > > > > Best regards, > Krzysztof >
On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote: > > + > > + phydev->drv->led_brightness_set = NULL; > > + phydev->drv->led_blink_set = NULL; > > + phydev->drv->led_hw_is_supported = NULL; > > + phydev->drv->led_hw_control_set = NULL; > > + phydev->drv->led_hw_control_get = NULL; > > I don't see how that works. You have multiple PHYs using this > driver. Some might have LEDs, some might have GPOs. But if you modify > the driver structure like this, you prevent all PHYs from having LEDs, > and maybe cause a Opps if a PHY device has already registered its > LEDs? > God you are right! Off-topic but given the effects this may cause, why the thing is not const? I assume it wouldn't make sense to add OPS based on the detected feature since it would have side effect on other PHYs that use the same driver.
On Fri, Feb 02, 2024 at 04:19:15PM +0100, Christian Marangi wrote: > On Fri, Feb 02, 2024 at 08:41:56AM +0100, Krzysztof Kozlowski wrote: > > On 01/02/2024 16:17, Christian Marangi wrote: > > > From: Robert Marko <robert.marko@sartura.hr> > > > > > > Add DT bindings defined for Qualcomm QCA807x PHY series related to > > > calibration and DAC settings. > > > > Nothing from this file is used and your commit msg does not provide > > rationale "why", thus it does not look like something for bindings. > > Otherwise please point me which patch with *driver* uses these bindings. > > > > Hi, since I have to squash this, I will include the reason in the schema > patch. > > Anyway these are raw values used to configure the qcom,control-dac > property. Maybe I am missing something, but a quick scan of the patchset and a grep of the tree doesn't show this property being documented anywhere. > In the driver it's used by qca807x_config_init. We read what is set in > DT and we configure the reg accordingly. > > If this is wrong should we use a more schema friendly approach with > declaring an enum of string and document that there? Without any idea of what that property is used for it is hard to say, but personally I much prefer enums of strings for what looks like a property that holds register values. That you felt it necessary to add defines for the values makes it more like that that is the case. Cheers, Conor. > > > > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > > --- > > > include/dt-bindings/net/qcom-qca807x.h | 30 ++++++++++++++++++++++++++ > > > > Use filename matching compatible, so vendor,device. No wildcards, unless > > your compatible also has them. > > > > > 1 file changed, 30 insertions(+) > > > create mode 100644 include/dt-bindings/net/qcom-qca807x.h > > > > > > > > > > > Best regards, > > Krzysztof > > > > -- > Ansuel
On Fri, Feb 02, 2024 at 04:58:32PM +0000, Conor Dooley wrote: > On Fri, Feb 02, 2024 at 04:19:15PM +0100, Christian Marangi wrote: > > On Fri, Feb 02, 2024 at 08:41:56AM +0100, Krzysztof Kozlowski wrote: > > > On 01/02/2024 16:17, Christian Marangi wrote: > > > > From: Robert Marko <robert.marko@sartura.hr> > > > > > > > > Add DT bindings defined for Qualcomm QCA807x PHY series related to > > > > calibration and DAC settings. > > > > > > Nothing from this file is used and your commit msg does not provide > > > rationale "why", thus it does not look like something for bindings. > > > Otherwise please point me which patch with *driver* uses these bindings. > > > > > > > Hi, since I have to squash this, I will include the reason in the schema > > patch. > > > > Anyway these are raw values used to configure the qcom,control-dac > > property. > > Maybe I am missing something, but a quick scan of the patchset and a > grep of the tree doesn't show this property being documented anywhere. > > > In the driver it's used by qca807x_config_init. We read what is set in > > DT and we configure the reg accordingly. > > > > If this is wrong should we use a more schema friendly approach with > > declaring an enum of string and document that there? > > Without any idea of what that property is used for it is hard to say, > but personally I much prefer enums of strings for what looks like a > property that holds register values. > > That you felt it necessary to add defines for the values makes it more > like that that is the case. > Ok, no problem. The big idea here was really to skip having to have a big function that parse strings but I get that it's better handle with string and have them documented directly in the yaml. > > > > > > > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > > > --- > > > > include/dt-bindings/net/qcom-qca807x.h | 30 ++++++++++++++++++++++++++ > > > > > > Use filename matching compatible, so vendor,device. No wildcards, unless > > > your compatible also has them. > > > > > > > 1 file changed, 30 insertions(+) > > > > create mode 100644 include/dt-bindings/net/qcom-qca807x.h > > > > > > > > > > > > > > > > Best regards, > > > Krzysztof > > > > > > > -- > > Ansuel
On Fri, Feb 02, 2024 at 05:40:21PM +0100, Christian Marangi wrote: > On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote: > > > + > > > + phydev->drv->led_brightness_set = NULL; > > > + phydev->drv->led_blink_set = NULL; > > > + phydev->drv->led_hw_is_supported = NULL; > > > + phydev->drv->led_hw_control_set = NULL; > > > + phydev->drv->led_hw_control_get = NULL; > > > > I don't see how that works. You have multiple PHYs using this > > driver. Some might have LEDs, some might have GPOs. But if you modify > > the driver structure like this, you prevent all PHYs from having LEDs, > > and maybe cause a Opps if a PHY device has already registered its > > LEDs? > > > > God you are right! Off-topic but given the effects this may cause, why > the thing is not const? I assume it wouldn't make sense to add OPS based > on the detected feature since it would have side effect on other PHYs > that use the same driver. Maybe phydev->drv should be const to avoid this kind of thing. It doesn't look like it would be hard to do, and importantly doesn't require casting away the const-ness anywhere. PHY drivers themselves can't be const because the driver model needs to be able to modify the embedded device_driver struct (e.g. see bus_add_driver().) drivers/net/phy/phy.c | 3 +-- drivers/net/phy/phy_device.c | 4 ++-- drivers/net/phy/xilinx_gmii2rgmii.c | 2 +- include/linux/phy.h | 2 +- 4 files changed, 5 insertions(+), 6 deletions(-) Just build-testing it.
On Fri, Feb 02, 2024 at 05:04:23PM +0000, Russell King (Oracle) wrote: > On Fri, Feb 02, 2024 at 05:40:21PM +0100, Christian Marangi wrote: > > On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote: > > > > + > > > > + phydev->drv->led_brightness_set = NULL; > > > > + phydev->drv->led_blink_set = NULL; > > > > + phydev->drv->led_hw_is_supported = NULL; > > > > + phydev->drv->led_hw_control_set = NULL; > > > > + phydev->drv->led_hw_control_get = NULL; > > > > > > I don't see how that works. You have multiple PHYs using this > > > driver. Some might have LEDs, some might have GPOs. But if you modify > > > the driver structure like this, you prevent all PHYs from having LEDs, > > > and maybe cause a Opps if a PHY device has already registered its > > > LEDs? > > > > > > > God you are right! Off-topic but given the effects this may cause, why > > the thing is not const? I assume it wouldn't make sense to add OPS based > > on the detected feature since it would have side effect on other PHYs > > that use the same driver. > > Maybe phydev->drv should be const to avoid this kind of thing. It > doesn't look like it would be hard to do, and importantly doesn't > require casting away the const-ness anywhere. PHY drivers themselves > can't be const because the driver model needs to be able to modify > the embedded device_driver struct (e.g. see bus_add_driver().) > > drivers/net/phy/phy.c | 3 +-- > drivers/net/phy/phy_device.c | 4 ++-- > drivers/net/phy/xilinx_gmii2rgmii.c | 2 +- > include/linux/phy.h | 2 +- > 4 files changed, 5 insertions(+), 6 deletions(-) > > Just build-testing it. > Seems sensible to me. Also for everyone that does that (downstream or driver that needs to be handled) it would result in a warning for modifying const stuff. Maybe I'm wrong but I can only see benefits in doing this change.
On Fri, Feb 02, 2024 at 05:40:21PM +0100, Christian Marangi wrote: > On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote: > > > + > > > + phydev->drv->led_brightness_set = NULL; > > > + phydev->drv->led_blink_set = NULL; > > > + phydev->drv->led_hw_is_supported = NULL; > > > + phydev->drv->led_hw_control_set = NULL; > > > + phydev->drv->led_hw_control_get = NULL; > > > > I don't see how that works. You have multiple PHYs using this > > driver. Some might have LEDs, some might have GPOs. But if you modify > > the driver structure like this, you prevent all PHYs from having LEDs, > > and maybe cause a Opps if a PHY device has already registered its > > LEDs? > > > > God you are right! Off-topic but given the effects this may cause, why > the thing is not const? I would like it to be, but its not easy. There are fields in the driver structure that phylib needs to modify. e.g. mdiodrv.driver gets passed to the driver core when registering the driver, and it modifies it. mdiodrv.flags is also manipulated. So we cannot make the whole structure const. Andrew
On Fri, Feb 02, 2024 at 06:08:33PM +0100, Andrew Lunn wrote: > On Fri, Feb 02, 2024 at 05:40:21PM +0100, Christian Marangi wrote: > > On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote: > > > > + > > > > + phydev->drv->led_brightness_set = NULL; > > > > + phydev->drv->led_blink_set = NULL; > > > > + phydev->drv->led_hw_is_supported = NULL; > > > > + phydev->drv->led_hw_control_set = NULL; > > > > + phydev->drv->led_hw_control_get = NULL; > > > > > > I don't see how that works. You have multiple PHYs using this > > > driver. Some might have LEDs, some might have GPOs. But if you modify > > > the driver structure like this, you prevent all PHYs from having LEDs, > > > and maybe cause a Opps if a PHY device has already registered its > > > LEDs? > > > > > > > God you are right! Off-topic but given the effects this may cause, why > > the thing is not const? > > I would like it to be, but its not easy. There are fields in the > driver structure that phylib needs to modify. e.g. mdiodrv.driver gets > passed to the driver core when registering the driver, and it modifies > it. mdiodrv.flags is also manipulated. So we cannot make the whole > structure const. > Maybe the ops part can be detached and just that made const? (and introduce something like struct phy_driver_ops) It would require a big conversion but assuming nobody adds OPs in probe function everything should be static and easy to convert.
On Fri, Feb 02, 2024 at 06:08:33PM +0100, Andrew Lunn wrote: > On Fri, Feb 02, 2024 at 05:40:21PM +0100, Christian Marangi wrote: > > On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote: > > > > + > > > > + phydev->drv->led_brightness_set = NULL; > > > > + phydev->drv->led_blink_set = NULL; > > > > + phydev->drv->led_hw_is_supported = NULL; > > > > + phydev->drv->led_hw_control_set = NULL; > > > > + phydev->drv->led_hw_control_get = NULL; > > > > > > I don't see how that works. You have multiple PHYs using this > > > driver. Some might have LEDs, some might have GPOs. But if you modify > > > the driver structure like this, you prevent all PHYs from having LEDs, > > > and maybe cause a Opps if a PHY device has already registered its > > > LEDs? > > > > > > > God you are right! Off-topic but given the effects this may cause, why > > the thing is not const? > > I would like it to be, but its not easy. There are fields in the > driver structure that phylib needs to modify. e.g. mdiodrv.driver gets > passed to the driver core when registering the driver, and it modifies > it. mdiodrv.flags is also manipulated. So we cannot make the whole > structure const. We can make phy_device's drv pointer const though, which would have the effect of catching code such as the above. That doesn't impact the driver model nor the mdio layer.
On Fri, Feb 02, 2024 at 02:35:11AM +0100, Andrew Lunn wrote: > > +static int qca807x_read_fiber_status(struct phy_device *phydev) > > +{ > > + int ss, err, lpa, old_link = phydev->link; > > + > > + /* 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(). Can it be used? > Yes but I had to expand genphy_c37_read_status. Hope it will be OK. > > + > > + /* Read the QCA807x PHY-Specific Status register fiber page, > > + * which indicates the speed and duplex that the PHY is actually > > + * using, irrespective of whether we are in autoneg mode or not. > > + */ > > + ss = phy_read(phydev, AT803X_SPECIFIC_STATUS); > > + if (ss < 0) > > + return ss; > > + > > + if (ss & AT803X_SS_SPEED_DUPLEX_RESOLVED) { > > + switch (FIELD_GET(AT803X_SS_SPEED_MASK, ss)) { > > + case AT803X_SS_SPEED_100: > > + phydev->speed = SPEED_100; > > + break; > > + case AT803X_SS_SPEED_1000: > > + phydev->speed = SPEED_1000; > > + break; > > + } > > + > > + if (ss & AT803X_SS_DUPLEX) > > + phydev->duplex = DUPLEX_FULL; > > + else > > + phydev->duplex = DUPLEX_HALF; > > + } > > + > > + return 0; > > +} > > > > +static int qca807x_phy_package_probe_once(struct phy_device *phydev) > > +{ > > + struct phy_package_shared *shared = phydev->shared; > > + struct qca807x_shared_priv *priv = shared->priv; > > + unsigned int tx_driver_strength = 0; > > + const char *package_mode_name; > > + > > + of_property_read_u32(shared->np, "qcom,tx-driver-strength", > > + &tx_driver_strength); > > + switch (tx_driver_strength) { > > + case 140: > > + priv->tx_driver_strength = PQSGMII_TX_DRIVER_140MV; > > + break; > > + case 160: > > + priv->tx_driver_strength = PQSGMII_TX_DRIVER_160MV; > > + break; > > + case 180: > > + priv->tx_driver_strength = PQSGMII_TX_DRIVER_180MV; > > + break; > > + case 200: > > ... > > > + case 500: > > + priv->tx_driver_strength = PQSGMII_TX_DRIVER_500MV; > > + break; > > + case 600: > > + default: > > If its missing default to 600. But if its an invalid value, return > -EINVAL. > > > + priv->tx_driver_strength = PQSGMII_TX_DRIVER_600MV; > > + } > > + > > + priv->package_mode = PHY_INTERFACE_MODE_NA; > > + if (!of_property_read_string(shared->np, "qcom,package-mode", > > + &package_mode_name)) { > > + if (!strcasecmp(package_mode_name, > > + phy_modes(PHY_INTERFACE_MODE_PSGMII))) > > + priv->package_mode = PHY_INTERFACE_MODE_PSGMII; > > + > > + if (!strcasecmp(package_mode_name, > > + phy_modes(PHY_INTERFACE_MODE_QSGMII))) > > + priv->package_mode = PHY_INTERFACE_MODE_QSGMII; > > Again, return -EINVAL if it is neither. > > > +static int qca807x_phy_package_config_init_once(struct phy_device *phydev) > > +{ > > + struct phy_package_shared *shared = phydev->shared; > > + struct qca807x_shared_priv *priv = shared->priv; > > + int val, ret; > > + > > + phy_lock_mdio_bus(phydev); > > + > > + /* Set correct PHY package mode */ > > + val = __phy_package_read(phydev, QCA807X_COMBO_ADDR, > > + QCA807X_CHIP_CONFIGURATION); > > + val &= ~QCA807X_CHIP_CONFIGURATION_MODE_CFG_MASK; > > + if (priv->package_mode == PHY_INTERFACE_MODE_QSGMII) > > + val |= QCA807X_CHIP_CONFIGURATION_MODE_QSGMII_SGMII; > > + else > > + val |= QCA807X_CHIP_CONFIGURATION_MODE_PSGMII_ALL_COPPER; > > What about priv->package_mode == PHY_INTERFACE_MODE_NA; > I changed this to a switch and added some comments to make this more clear. We default to PSGMII if package-mode is not defined. (will also update schema with default value)
On Thu, Feb 01, 2024 at 04:17:27PM +0100, Christian Marangi wrote: > Document ethernet PHY package nodes used to describe PHY shipped in > bundle of 2-5 PHY. The special node describe a container of PHY that > share common properties. This is a generic schema and PHY package > should create specialized version with the required additional shared > properties. > > Example are PHY packages that have some regs only in one PHY of the > package and will affect every other PHY in the package, for example > related to PHY interface mode calibration or global PHY mode selection. > > The PHY package node MUST declare the base address used by the PHY driver > for global configuration by calculating the offsets of the global PHY > based on the base address of the PHY package. > > Each reg of the PHYs defined in the PHY Package node is an offset of the > PHY Package reg. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > .../bindings/net/ethernet-phy-package.yaml | 55 +++++++++++++++++++ > 1 file changed, 55 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/ethernet-phy-package.yaml > > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml > new file mode 100644 > index 000000000000..d7cdbb1a4b3e > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml > @@ -0,0 +1,55 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/ethernet-phy-package.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Ethernet PHY Package Common Properties > + > +maintainers: > + - Christian Marangi <ansuelsmth@gmail.com> > + > +description: > + PHY packages are multi-port Ethernet PHY of the same family > + and each Ethernet PHY is affected by the global configuration > + of the PHY package. > + > + Each reg of the PHYs defined in the PHY Package node is > + an offset of the PHY Package reg. > + > + Each Ethernet PHYs defined in the PHY package node is > + reachable in the MDIO bus at the address of the PHY > + Package offset of the Ethernet PHY reg. If the phys are addressed with an MDIO address, then just use those. > + > +properties: > + $nodename: > + pattern: "^ethernet-phy-package(@[a-f0-9]+)?$" Can't be optional if 'reg' is required (which it should be). > + > + reg: > + minimum: 0 > + maximum: 31 > + description: > + The base ID number for the PHY package. > + Commonly the ID of the first PHY in the PHY package. > + > + Some PHY in the PHY package might be not defined but > + still occupy ID on the device (just not attached to > + anything) hence the PHY package reg might correspond > + to a not attached PHY (offset 0). > + > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > +patternProperties: > + ^ethernet-phy(@[a-f0-9]+)?$: Same issue here. > + $ref: ethernet-phy.yaml# > + > +required: > + - reg > + - '#address-cells' > + - '#size-cells' > + > +additionalProperties: true > -- > 2.43.0 >
On Fri, Feb 02, 2024 at 02:52:59PM -0600, Rob Herring wrote: > On Thu, Feb 01, 2024 at 04:17:27PM +0100, Christian Marangi wrote: > > Document ethernet PHY package nodes used to describe PHY shipped in > > bundle of 2-5 PHY. The special node describe a container of PHY that > > share common properties. This is a generic schema and PHY package > > should create specialized version with the required additional shared > > properties. > > > > Example are PHY packages that have some regs only in one PHY of the > > package and will affect every other PHY in the package, for example > > related to PHY interface mode calibration or global PHY mode selection. > > > > The PHY package node MUST declare the base address used by the PHY driver > > for global configuration by calculating the offsets of the global PHY > > based on the base address of the PHY package. > > > > Each reg of the PHYs defined in the PHY Package node is an offset of the > > PHY Package reg. > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > --- > > .../bindings/net/ethernet-phy-package.yaml | 55 +++++++++++++++++++ > > 1 file changed, 55 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/net/ethernet-phy-package.yaml > > > > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml > > new file mode 100644 > > index 000000000000..d7cdbb1a4b3e > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml > > @@ -0,0 +1,55 @@ > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/net/ethernet-phy-package.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Ethernet PHY Package Common Properties > > + > > +maintainers: > > + - Christian Marangi <ansuelsmth@gmail.com> > > + > > +description: > > + PHY packages are multi-port Ethernet PHY of the same family > > + and each Ethernet PHY is affected by the global configuration > > + of the PHY package. > > + > > + Each reg of the PHYs defined in the PHY Package node is > > + an offset of the PHY Package reg. > > + > > + Each Ethernet PHYs defined in the PHY package node is > > + reachable in the MDIO bus at the address of the PHY > > + Package offset of the Ethernet PHY reg. > > If the phys are addressed with an MDIO address, then just use those. > Thanks a lot for the review Rob. Just to make sure I got this right! Are you ok to have the PHYs with absolute reg? (it would make thing soo much easy) It would result in this node example on real world scenario. ethernet-phy-package@16 { compatible = "qcom,qca8075-package"; #address-cells = <1>; #size-cells = <0>; reg = <16>; qcom,package-mode = "qsgmii"; qca8075_16: ethernet-phy@16 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <16>; }; qca8075_17: ethernet-phy@17 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <17>; }; qca8075_18: ethernet-phy@18 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <18>; }; qca8075_19: ethernet-phy@19 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <19>; }; }; > > + > > +properties: > > + $nodename: > > + pattern: "^ethernet-phy-package(@[a-f0-9]+)?$" > > Can't be optional if 'reg' is required (which it should be). > > > + > > + reg: > > + minimum: 0 > > + maximum: 31 > > + description: > > + The base ID number for the PHY package. > > + Commonly the ID of the first PHY in the PHY package. > > + > > + Some PHY in the PHY package might be not defined but > > + still occupy ID on the device (just not attached to > > + anything) hence the PHY package reg might correspond > > + to a not attached PHY (offset 0). > > + > > + '#address-cells': > > + const: 1 > > + > > + '#size-cells': > > + const: 0 > > + > > +patternProperties: > > + ^ethernet-phy(@[a-f0-9]+)?$: > > Same issue here. > > > + $ref: ethernet-phy.yaml# > > + > > +required: > > + - reg > > + - '#address-cells' > > + - '#size-cells' > > + > > +additionalProperties: true > > -- > > 2.43.0 > >
On Fri, Feb 02, 2024 at 06:44:22PM +0100, Christian Marangi wrote: > On Fri, Feb 02, 2024 at 02:35:11AM +0100, Andrew Lunn wrote: > > > +static int qca807x_read_fiber_status(struct phy_device *phydev) > > > +{ > > > + int ss, err, lpa, old_link = phydev->link; > > > + > > > + /* 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(). Can it be used? > > > > Yes but I had to expand genphy_c37_read_status. Hope it will be OK. You can expand it, but please keep to what is defined within 802.3. We don't want any vendor extensions in this common code. Vendor things should be kept in the vendor driver. So you can call genphy_c37_read_status() and then do any vendor specific fixups needed. Andrew
On Sat, Feb 03, 2024 at 05:25:23PM +0100, Andrew Lunn wrote: > On Fri, Feb 02, 2024 at 06:44:22PM +0100, Christian Marangi wrote: > > On Fri, Feb 02, 2024 at 02:35:11AM +0100, Andrew Lunn wrote: > > > > +static int qca807x_read_fiber_status(struct phy_device *phydev) > > > > +{ > > > > + int ss, err, lpa, old_link = phydev->link; > > > > + > > > > + /* 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(). Can it be used? > > > > > > > Yes but I had to expand genphy_c37_read_status. Hope it will be OK. > > You can expand it, but please keep to what is defined within 802.3. We > don't want any vendor extensions in this common code. Vendor things > should be kept in the vendor driver. So you can call > genphy_c37_read_status() and then do any vendor specific fixups > needed. > Sure the expansion is just adding a bool signal if the link has changed or not (to make it possible to exit early and skip the additional vendor call...) I didn't add anything to the c37 function ifself. Anyway of from this, the revision is ready, just need to understand if Rob is ok with absolute or relative address for PHYs in the PHY package node.
Idea of this big series is to introduce the concept of PHY package in DT and give PHY drivers a way to derive the base address from DT. The concept of PHY package is nothing new and is already a thing in the kernel with the API phy_package_join/leave/read/write. What is currently lacking is describing this in DT and better reference a base address to calculate offset from. In the scenario of a PHY package where multiple address are used and there isn't a way to get the base address of the PHY package from some regs, getting the information from DT is the only way. A possible example to this problem is this: ethernet-phy-package@0 { compatible = "qcom,qca807x-package"; #address-cells = <1>; #size-cells = <0>; reg = <0>; qcom,package-mode = "qsgmii"; ethernet-phy@1 { reg = <1>; }; phy4: ethernet-phy@4 { reg = <4>; }; }; The mdio parse functions are changed to address for this additional special node, the function is changed to simply detect this node and search also in this. (we match the node name to be "ethernet-phy-package") PHY driver can then use introduced helper of_phy_package_join to join the PHY to the PHY package and derive the base address from DT. The base addr + offset implementation adds additional complexity to the code but I think will make DT reviwers happier since the absolute reg implementation might make things confusing with having double reg in the DTS. I'm open to any alternative implementation and also to revert this to the absolute implementation. Changes v5: - Rebase on top of net-next - Change implementation to base addr + offset in subnode - Adapt to all the changes and cleanup done to at803x Changes v4: - Rework DT implementation - Drop of autojoin support and rework to simple helper - Rework PHY driver to the new implementation - Add compatible for qca807x package - Further cleanup patches Changes v3: - Add back compatible implementation - Detach patch that can be handled separately (phy_package_mmd, phy_package extended) - Rework code to new simplified implementation with base addr + offset - Improve documentation with additional info and description Changes v2: - Drop compatible "ethernet-phy-package", use node name prefix matching instead - Improve DT example - Add reg for ethernet-phy-package - Drop phy-mode for ethernet-phy-package - Drop patch for generalization of phy-mode - Drop global-phy property (handle internally to the PHY driver) - Rework OF phy package code and PHY driver to handle base address - Fix missing of_node_put - Add some missing docs for added variables in struct - Move some define from dt-bindings include to PHY driver - Handle qsgmii validation in PHY driver - Fix wrong include for gpiolib - Drop reduntant version.h include Christian Marangi (7): dt-bindings: net: document ethernet PHY package nodes net: phy: add support for scanning PHY in PHY packages nodes net: phy: add devm/of_phy_package_join helper net: phy: qcom: move more function to shared library dt-bindings: net: Document Qcom QCA807x PHY package net: phy: qcom: generalize some qca808x LED functions net: phy: qca807x: add support for configurable LED Robert Marko (2): dt-bindings: net: add QCA807x PHY defines net: phy: qcom: add support for QCA807x PHY Family .../bindings/net/ethernet-phy-package.yaml | 55 ++ .../devicetree/bindings/net/qcom,qca807x.yaml | 142 +++ drivers/net/mdio/of_mdio.c | 75 +- drivers/net/phy/mdio_bus.c | 44 +- drivers/net/phy/phy_device.c | 84 ++ drivers/net/phy/qcom/Kconfig | 8 + drivers/net/phy/qcom/Makefile | 1 + drivers/net/phy/qcom/at803x.c | 35 - drivers/net/phy/qcom/qca807x.c | 832 ++++++++++++++++++ drivers/net/phy/qcom/qca808x.c | 311 +------ drivers/net/phy/qcom/qcom-phy-lib.c | 247 ++++++ drivers/net/phy/qcom/qcom.h | 123 +++ include/dt-bindings/net/qcom-qca807x.h | 30 + include/linux/of_mdio.h | 26 + include/linux/phy.h | 6 + 15 files changed, 1648 insertions(+), 371 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/ethernet-phy-package.yaml create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml create mode 100644 drivers/net/phy/qcom/qca807x.c create mode 100644 include/dt-bindings/net/qcom-qca807x.h