net: sfp: improve high power module implementation
Subsystems (5)
Russell King (Oracle)
maintainerv2 cover net: sfp: improve high power module implementation
Hi,
This series aims to improve the power level switching between standard level 1 and the higher power levels.
The first patch updates the DT binding documentation to include the minimum and default of 1W, which is the base level that every SFP cage must support. Hence, it makes sense to document this in the binding.
The second patch enforces a minimum of 1W when parsing the firmware description, and optimises the code for that case; there's no need to check for SFF8472 compliance since we will not need to touch the A2h registers.
Patch 3 validates that the module supports SFF-8472 rev 10.2 before checking for power level 2 - rev 10.2 is where support for power levels was introduced, so if the module doesn't support this revision, it doesn't support power levels. Setting the power level 2 declaration bit is likely to be spurious.
Patch 4 does the same for power level 3, except this was introduced in SFF-8472 rev 11.9. The revision code was never updated, so we use the rev 11.4 to signify this.
Patch 5 cleans up the code - rather than using BIT(0), we now use a properly named value for the power level select bit.
Patch 6 introduces a read-modify-write helper.
Patch 7 gets rid of the DM7052 hack (which sets a power level declaration bit but is not compatible with SFF-8472 rev 10.2, and the module does not implement the A2h I2C address.)
Series tested with my DM7052.
v2: update sff.sfp.yaml with Rob's feedback
Documentation/devicetree/bindings/net/sff,sfp.yaml | 3 +- drivers/net/phy/sfp.c | 85 +++++++++++----------- include/linux/sfp.h | 2 + 3 files changed, 48 insertions(+), 42 deletions(-)
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
PATCH 1/7 dt-bindings: net: sff,sfp: update binding
Add a minimum and default for the maximum-power-milliwatt option; module power levels were originally up to 1W, so this is the default and the minimum power level we can have for a functional SFP cage.
--- Documentation/devicetree/bindings/net/sff,sfp.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/net/sff,sfp.yaml +2 -1
@@ -22,7 +22,8 @@ title: Small Form Factor (SFF) Committee Small Form-factor Pluggable (SFP) phandle of an I2C bus controller for the SFP two wire serial maximum-power-milliwatt: - maxItems: 1 + minimum: 1000 + default: 1000 description: Maximum module power consumption Specifies the maximum power consumption allowable by a module in the slot, in milli-Watts. Presently, modules can
Signature · 1 line
2.30.2
PATCH 2/7 net: sfp: check firmware provided max power
In reply to Russell King (Oracle)Check that the firmware provided maximum power is at least 1W, which iis the minimum power level for any SFP module.
Now that we enforce the minimum of 1W, we can exit early from sfp_module_parse_power() if the module power is 1W or less.
--- drivers/net/phy/sfp.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/net/phy/sfp.c +11 -7
@@ -1766,6 +1766,12 @@ static int sfp_module_parse_power(struct sfp *sfp)if (sfp->id.ext.options &cpu_to_be16 (SFP_OPTIONS_HIGH_POWER_LEVEL )) power_mW =2000 ; +/* Power level 1 modules (max. 1W) are always supported. */ +if (power_mW <=1000 ) { + sfp->module_power_mW = power_mW; +return 0 ; + } + supports_a2 = sfp->id.ext.sff8472_compliance !=SFP_SFF8472_COMPLIANCE_NONE || sfp->id.ext.diagmon &SFP_DIAGMON_DDM ;
@@ -1789,12 +1795,6 @@ static int sfp_module_parse_power(struct sfp *sfp) } } -if (power_mW <=1000 ) { -/* Modules below 1W do not require a power change sequence */ - sfp->module_power_mW = power_mW; -return 0 ; - } -if (!supports_a2) {/* The module power level is below the host maximum and the * module appears not to implement bus address 0xa2, so assume
@@ -2729,8 +2729,12 @@ static int sfp_probe(struct platform_device *pdev)device_property_read_u32 (&pdev->dev,"maximum-power-milliwatt" , &sfp->max_power_mW); -if (!sfp->max_power_mW) +if (sfp->max_power_mW <1000 ) { +if (sfp->max_power_mW) +dev_warn (sfp->dev, +"Firmware bug: host maximum power should be at least 1W\n" ); sfp->max_power_mW =1000 ; + }dev_info (sfp->dev,"Host maximum power %u.%uW\n" , sfp->max_power_mW /1000 , (sfp->max_power_mW /100 ) %10 );
Signature · 1 line
2.30.2
PATCH 3/7 net: sfp: ignore power level 2 prior to SFF-8472 Rev 10.2
In reply to Russell King (Oracle)Power level 2 was introduced by SFF-8472 revision 10.2. Ignore the power declaration bit for modules that are not compliant with at least this revision.
This should remove any spurious indication of 1.5W modules.
--- drivers/net/phy/sfp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/phy/sfp.c +2 -1
@@ -1761,7 +1761,8 @@ static int sfp_module_parse_power(struct sfp *sfp)u32 power_mW =1000 ;bool supports_a2; -if (sfp->id.ext.options &cpu_to_be16 (SFP_OPTIONS_POWER_DECL )) +if (sfp->id.ext.sff8472_compliance >=SFP_SFF8472_COMPLIANCE_REV10_2 && + sfp->id.ext.options &cpu_to_be16 (SFP_OPTIONS_POWER_DECL )) power_mW =1500 ;if (sfp->id.ext.options &cpu_to_be16 (SFP_OPTIONS_HIGH_POWER_LEVEL )) power_mW =2000 ;
Signature · 1 line
2.30.2
PATCH 4/7 net: sfp: ignore power level 3 prior to SFF-8472 Rev 11.4
In reply to Russell King (Oracle)Power level 3 was included in SFF-8472 revision 11.9, but this does not have a compliance code. Use revision 11.4 as the minimum compliance level instead.
This should avoid any spurious indication of 2W modules.
--- drivers/net/phy/sfp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/net/phy/sfp.c +3 -1
@@ -1764,7 +1764,9 @@ static int sfp_module_parse_power(struct sfp *sfp)if (sfp->id.ext.sff8472_compliance >=SFP_SFF8472_COMPLIANCE_REV10_2 && sfp->id.ext.options &cpu_to_be16 (SFP_OPTIONS_POWER_DECL )) power_mW =1500 ; -if (sfp->id.ext.options &cpu_to_be16 (SFP_OPTIONS_HIGH_POWER_LEVEL )) +/* Added in Rev 11.9, but there is no compliance code for this */ +if (sfp->id.ext.sff8472_compliance >=SFP_SFF8472_COMPLIANCE_REV11_4 && + sfp->id.ext.options &cpu_to_be16 (SFP_OPTIONS_HIGH_POWER_LEVEL )) power_mW =2000 ;/* Power level 1 modules (max. 1W) are always supported. */
Signature · 1 line
2.30.2
PATCH 5/7 net: sfp: provide a definition for the power level select bit
In reply to Russell King (Oracle)Provide a named definition for the power level select bit in the extended status register, rather than using BIT(0) in the code.
--- drivers/net/phy/sfp.c | 6 +++--- include/linux/sfp.h | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-)
drivers/net/phy/sfp.c +3 -3
@@ -1837,13 +1837,13 @@ static int sfp_sm_mod_hpower(struct sfp *sfp, bool enable)* all bytes 0xff) at 0x51 but does not accept writes. In any case, * if the bit is already set, we're already in high power mode. */ -if (!!(val &BIT (0 )) == enable) +if (!!(val &SFP_EXT_STATUS_PWRLVL_SELECT ) == enable)return 0 ;if (enable) - val |=BIT (0 ); + val |=SFP_EXT_STATUS_PWRLVL_SELECT ;else - val &= ~BIT (0 ); + val &= ~SFP_EXT_STATUS_PWRLVL_SELECT ; err =sfp_write (sfp, true,SFP_EXT_STATUS , &val,sizeof (val));if (err !=sizeof (val)) {
include/linux/sfp.h +2 -0
@@ -489,6 +489,8 @@ enum {SFP_WARN1_RXPWR_LOW =BIT (6 ),SFP_EXT_STATUS =0x76 , +SFP_EXT_STATUS_PWRLVL_SELECT =BIT (0 ), +SFP_VSL =0x78 , SFP_PAGE = 0x7f, };
Signature · 1 line
2.30.2
PATCH 6/7 net: sfp: add sfp_modify_u8() helper
In reply to Russell King (Oracle)Add a helper to modify bits in a single byte in memory space, and use it when updating the soft tx-disable flag in the module.
--- drivers/net/phy/sfp.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)
drivers/net/phy/sfp.c +22 -9
@@ -608,6 +608,22 @@ static int sfp_write(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len)return sfp->write (sfp, a2, addr, buf, len); } +static int sfp_modify_u8 (struct sfp *sfp,bool a2,u8 addr,u8 mask,u8 val) +{ +int ret; +u8 old, v; + + ret =sfp_read (sfp, a2, addr, &old,sizeof (old)); +if (ret !=sizeof (old)) +return ret; + + v = (old & ~mask) | (val & mask); +if (v == old) +return sizeof (v); + +return sfp_write (sfp, a2, addr, &v,sizeof (v)); +} +static unsigned int sfp_soft_get_state (struct sfp *sfp) {unsigned int state =0 ;
@@ -633,17 +649,14 @@ static unsigned int sfp_soft_get_state(struct sfp *sfp)static void sfp_soft_set_state (struct sfp *sfp,unsigned int state) { -u8 status; +u8 mask =SFP_STATUS_TX_DISABLE_FORCE ; +u8 val =0 ; -if (sfp_read (sfp, true,SFP_STATUS , &status,sizeof (status)) == -sizeof (status)) { -if (state &SFP_F_TX_DISABLE ) - status |=SFP_STATUS_TX_DISABLE_FORCE ; -else - status &= ~SFP_STATUS_TX_DISABLE_FORCE ; +if (state &SFP_F_TX_DISABLE ) + val |=SFP_STATUS_TX_DISABLE_FORCE ; -sfp_write (sfp, true,SFP_STATUS , &status,sizeof (status)); - } + +sfp_modify_u8 (sfp, true,SFP_STATUS , mask, val); }static void sfp_soft_start_poll (struct sfp *sfp)
Signature · 1 line
2.30.2
PATCH 7/7 net: sfp: get rid of DM7052 hack when enabling high power
In reply to Russell King (Oracle)Since we no longer mis-detect high-power mode with the DM7052 module, we no longer need the hack in sfp_module_enable_high_power(), and can now switch this to use sfp_modify_u8().
--- drivers/net/phy/sfp.c | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-)
drivers/net/phy/sfp.c +6 -23
@@ -1837,31 +1837,14 @@ static int sfp_module_parse_power(struct sfp *sfp)static int sfp_sm_mod_hpower (struct sfp *sfp,bool enable) { -u8 val;int err; - err =sfp_read (sfp, true,SFP_EXT_STATUS , &val,sizeof (val)); -if (err !=sizeof (val)) { -dev_err (sfp->dev,"Failed to read EEPROM: %pe\n" ,ERR_PTR (err)); -return -EAGAIN ; - } - -/* DM7052 reports as a high power module, responds to reads (with -* all bytes 0xff) at 0x51 but does not accept writes. In any case, -* if the bit is already set, we're already in high power mode. -*/ -if (!!(val &SFP_EXT_STATUS_PWRLVL_SELECT ) == enable) -return 0 ; - -if (enable) - val |=SFP_EXT_STATUS_PWRLVL_SELECT ; -else - val &= ~SFP_EXT_STATUS_PWRLVL_SELECT ; - - err =sfp_write (sfp, true,SFP_EXT_STATUS , &val,sizeof (val)); -if (err !=sizeof (val)) { -dev_err (sfp->dev,"Failed to write EEPROM: %pe\n" , -ERR_PTR (err)); + err =sfp_modify_u8 (sfp, true,SFP_EXT_STATUS , +SFP_EXT_STATUS_PWRLVL_SELECT , + enable ?SFP_EXT_STATUS_PWRLVL_SELECT :0 ); +if (err !=sizeof (u8)) { +dev_err (sfp->dev,"failed to %sable high power: %pe\n" , + enable ?"en" :"dis" ,ERR_PTR (err));return -EAGAIN ; }
Signature · 1 line
2.30.2
Re: [PATCH net-next 1/7] dt-bindings: net: sff,sfp: update binding
In reply to Russell King (Oracle)Quoted 9 lines · Russell King (Oracle)
Add a minimum and default for the maximum-power-milliwatt option; module power levels were originally up to 1W, so this is the default and the minimum power level we can have for a functional SFP cage.
Signed-off-by Russell King (Oracle) <[email protected]>--- Documentation/devicetree/bindings/net/sff,sfp.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
This series was applied to netdev/net-next.git (master) by Jakub Kicinski <[email protected]>:
Hello:
This series was applied to netdev/net-next.git (master) by Jakub Kicinski <[email protected]>:
Quoted 10 lines · On Fri, 21 Oct 2022 16:08:29 +0100 you wrote:
Hi,
This series aims to improve the power level switching between standard level 1 and the higher power levels.
The first patch updates the DT binding documentation to include the minimum and default of 1W, which is the base level that every SFP cage must support. Hence, it makes sense to document this in the binding.
[...]
Here is the summary with links:
- [net-next,1/7] dt-bindings: net: sff,sfp: update binding
https://git.kernel.org/netdev/net-next/c/a272bcb9e5ef - [net-next,2/7] net: sfp: check firmware provided max power https://git.kernel.org/netdev/net-next/c/02eaf5a79100 - [net-next,3/7] net: sfp: ignore power level 2 prior to SFF-8472 Rev 10.2 https://git.kernel.org/netdev/net-next/c/18cc659e95ab - [net-next,4/7] net: sfp: ignore power level 3 prior to SFF-8472 Rev 11.4 https://git.kernel.org/netdev/net-next/c/f8810ca75829 - [net-next,5/7] net: sfp: provide a definition for the power level select bit https://git.kernel.org/netdev/net-next/c/398900498485 - [net-next,6/7] net: sfp: add sfp_modify_u8() helper https://git.kernel.org/netdev/net-next/c/a3c536fc7580 - [net-next,7/7] net: sfp: get rid of DM7052 hack when enabling high power https://git.kernel.org/netdev/net-next/c/bd1432f68ddc
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html