[PATCH net-next 0/7] net: sfp: improve high power module implementation

[PATCH net-next 0/7] net: sfp: improve high power module implementation

[Russell King (Oracle)]

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.

 Documentation/devicetree/bindings/net/sff,sfp.yaml |  2 +
 drivers/net/phy/sfp.c                              | 85 +++++++++++-----------
 include/linux/sfp.h                                |  2 +
 3 files changed, 48 insertions(+), 41 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next 1/7] dt-bindings: net: sff,sfp: update binding

[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) <rmk+kernel@armlinux.org.uk>
---
 Documentation/devicetree/bindings/net/sff,sfp.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/sff,sfp.yaml b/Documentation/devicetree/bindings/net/sff,sfp.yaml
index 06c66ab81c01..20d30cccc95e 100644
--- a/Documentation/devicetree/bindings/net/sff,sfp.yaml
+++ b/Documentation/devicetree/bindings/net/sff,sfp.yaml
@@ -23,6 +23,8 @@ title: Small Form Factor (SFF) Committee Small Form-factor Pluggable (SFP)
 
   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
-- 
2.30.2

[PATCH net-next 2/7] net: sfp: check firmware provided max power

[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.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 40c9a64c5e30..f7ad4d5d9041 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -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);
-- 
2.30.2

[PATCH net-next 3/7] net: sfp: ignore power level 2 prior to SFF-8472 Rev 10.2

[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.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index f7ad4d5d9041..a7635b02524a 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -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;
-- 
2.30.2

[PATCH net-next 4/7] net: sfp: ignore power level 3 prior to SFF-8472 Rev 11.4

[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.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index a7635b02524a..af676e28ba6a 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -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. */
-- 
2.30.2

[PATCH net-next 5/7] net: sfp: provide a definition for the power level select bit

[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.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp.c | 6 +++---
 include/linux/sfp.h   | 2 ++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index af676e28ba6a..16bce0ea68d9 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -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)) {
diff --git a/include/linux/sfp.h b/include/linux/sfp.h
index d1f343853b6c..01ae9f1dd2ad 100644
--- a/include/linux/sfp.h
+++ b/include/linux/sfp.h
@@ -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,
 };
-- 
2.30.2

[PATCH net-next 6/7] net: sfp: add sfp_modify_u8() helper

[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.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 16bce0ea68d9..921bbedd9b22 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -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)
-- 
2.30.2

[PATCH net-next 7/7] net: sfp: get rid of DM7052 hack when enabling high power

[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().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp.c | 29 ++++++-----------------------
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 921bbedd9b22..39fd1811375c 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -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;
 	}
 
-- 
2.30.2

Re: [PATCH net-next 1/7] dt-bindings: net: sff,sfp: update binding

[Rob Herring]

On Wed, 19 Oct 2022 14:28:46 +0100, Russell King (Oracle) wrote:
> 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) <rmk+kernel@armlinux.org.uk>
> ---
>  Documentation/devicetree/bindings/net/sff,sfp.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/sff,sfp.yaml: properties:maximum-power-milliwatt: 'minimum' should not be valid under {'enum': ['const', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'minimum', 'maximum', 'multipleOf', 'pattern']}
	hint: Scalar and array keywords cannot be mixed
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH net-next 1/7] dt-bindings: net: sff,sfp: update binding

[Russell King (Oracle)]

On Wed, Oct 19, 2022 at 06:31:53PM -0500, Rob Herring wrote:
> On Wed, 19 Oct 2022 14:28:46 +0100, Russell King (Oracle) wrote:
> > 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) <rmk+kernel@armlinux.org.uk>
> > ---
> >  Documentation/devicetree/bindings/net/sff,sfp.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/sff,sfp.yaml: properties:maximum-power-milliwatt: 'minimum' should not be valid under {'enum': ['const', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'minimum', 'maximum', 'multipleOf', 'pattern']}
> 	hint: Scalar and array keywords cannot be mixed
> 	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#

I'm reading that error message and it means absolutely nothing to me.
Please can you explain it (and also re-word it to be clearer)?

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/7] dt-bindings: net: sff,sfp: update binding

[Rob Herring]

On Thu, Oct 20, 2022 at 09:28:25AM +0100, Russell King (Oracle) wrote:
> On Wed, Oct 19, 2022 at 06:31:53PM -0500, Rob Herring wrote:
> > On Wed, 19 Oct 2022 14:28:46 +0100, Russell King (Oracle) wrote:
> > > 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) <rmk+kernel@armlinux.org.uk>
> > > ---
> > >  Documentation/devicetree/bindings/net/sff,sfp.yaml | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > 
> > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> > 
> > yamllint warnings/errors:
> > 
> > dtschema/dtc warnings/errors:
> > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/sff,sfp.yaml: properties:maximum-power-milliwatt: 'minimum' should not be valid under {'enum': ['const', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'minimum', 'maximum', 'multipleOf', 'pattern']}
> > 	hint: Scalar and array keywords cannot be mixed
> > 	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
> 
> I'm reading that error message and it means absolutely nothing to me.
> Please can you explain it (and also re-word it to be clearer)?

'maxItems' is a constraint for arrays. 'maximum' is a constraint for 
scalar values. Mixing them does not make sense.

I have little control over the 1st line as that comes from jsonschema 
package. 'hint' is what I've added to explain things a bit more.

Rob

Re: [PATCH net-next 1/7] dt-bindings: net: sff,sfp: update binding

[Rob Herring]

On Thu, Oct 20, 2022 at 9:19 AM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Oct 20, 2022 at 09:28:25AM +0100, Russell King (Oracle) wrote:
> > On Wed, Oct 19, 2022 at 06:31:53PM -0500, Rob Herring wrote:
> > > On Wed, 19 Oct 2022 14:28:46 +0100, Russell King (Oracle) wrote:
> > > > 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) <rmk+kernel@armlinux.org.uk>
> > > > ---
> > > >  Documentation/devicetree/bindings/net/sff,sfp.yaml | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > >
> > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> > >
> > > yamllint warnings/errors:
> > >
> > > dtschema/dtc warnings/errors:
> > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/sff,sfp.yaml: properties:maximum-power-milliwatt: 'minimum' should not be valid under {'enum': ['const', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'minimum', 'maximum', 'multipleOf', 'pattern']}
> > >     hint: Scalar and array keywords cannot be mixed
> > >     from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
> >
> > I'm reading that error message and it means absolutely nothing to me.
> > Please can you explain it (and also re-word it to be clearer)?
>
> 'maxItems' is a constraint for arrays. 'maximum' is a constraint for
> scalar values. Mixing them does not make sense.

TBC, dropping 'maxItems' is what is needed here.

Rob

Re: [PATCH net-next 1/7] dt-bindings: net: sff,sfp: update binding

[Russell King (Oracle)]

On Thu, Oct 20, 2022 at 09:19:23AM -0500, Rob Herring wrote:
> On Thu, Oct 20, 2022 at 09:28:25AM +0100, Russell King (Oracle) wrote:
> > On Wed, Oct 19, 2022 at 06:31:53PM -0500, Rob Herring wrote:
> > > On Wed, 19 Oct 2022 14:28:46 +0100, Russell King (Oracle) wrote:
> > > > 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) <rmk+kernel@armlinux.org.uk>
> > > > ---
> > > >  Documentation/devicetree/bindings/net/sff,sfp.yaml | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > 
> > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> > > 
> > > yamllint warnings/errors:
> > > 
> > > dtschema/dtc warnings/errors:
> > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/sff,sfp.yaml: properties:maximum-power-milliwatt: 'minimum' should not be valid under {'enum': ['const', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'minimum', 'maximum', 'multipleOf', 'pattern']}
> > > 	hint: Scalar and array keywords cannot be mixed
> > > 	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
> > 
> > I'm reading that error message and it means absolutely nothing to me.
> > Please can you explain it (and also re-word it to be clearer)?
> 
> 'maxItems' is a constraint for arrays. 'maximum' is a constraint for 
> scalar values. Mixing them does not make sense.
> 
> I have little control over the 1st line as that comes from jsonschema 
> package. 'hint' is what I've added to explain things a bit more.

Given that maximum-power-milliwatt is a single value and has never been
an array, it seems then that the original conversion to yaml was wrong.
What should it have been?

(I'm clueless what the difference is between an array and scalar in
this yaml stuff.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/7] dt-bindings: net: sff,sfp: update binding

[Russell King (Oracle)]

On Thu, Oct 20, 2022 at 09:27:44AM -0500, Rob Herring wrote:
> On Thu, Oct 20, 2022 at 9:19 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Oct 20, 2022 at 09:28:25AM +0100, Russell King (Oracle) wrote:
> > > On Wed, Oct 19, 2022 at 06:31:53PM -0500, Rob Herring wrote:
> > > > On Wed, 19 Oct 2022 14:28:46 +0100, Russell King (Oracle) wrote:
> > > > > 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) <rmk+kernel@armlinux.org.uk>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/net/sff,sfp.yaml | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > >
> > > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > > > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> > > >
> > > > yamllint warnings/errors:
> > > >
> > > > dtschema/dtc warnings/errors:
> > > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/sff,sfp.yaml: properties:maximum-power-milliwatt: 'minimum' should not be valid under {'enum': ['const', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'minimum', 'maximum', 'multipleOf', 'pattern']}
> > > >     hint: Scalar and array keywords cannot be mixed
> > > >     from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
> > >
> > > I'm reading that error message and it means absolutely nothing to me.
> > > Please can you explain it (and also re-word it to be clearer)?
> >
> > 'maxItems' is a constraint for arrays. 'maximum' is a constraint for
> > scalar values. Mixing them does not make sense.
> 
> TBC, dropping 'maxItems' is what is needed here.

So how does this work?

maxItems: 1

tells it that there should be an array of one property, which is at the
DT level fundamentally the same as a scalar property.

minimum:
default:
maximum:

tells it that this is a scalar property, so there should be exactly one
item or the property should not be mentioned?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/7] dt-bindings: net: sff,sfp: update binding

[Rob Herring]

On Thu, Oct 20, 2022 at 11:08 AM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Thu, Oct 20, 2022 at 09:27:44AM -0500, Rob Herring wrote:
> > On Thu, Oct 20, 2022 at 9:19 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Thu, Oct 20, 2022 at 09:28:25AM +0100, Russell King (Oracle) wrote:
> > > > On Wed, Oct 19, 2022 at 06:31:53PM -0500, Rob Herring wrote:
> > > > > On Wed, 19 Oct 2022 14:28:46 +0100, Russell King (Oracle) wrote:
> > > > > > 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) <rmk+kernel@armlinux.org.uk>
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/net/sff,sfp.yaml | 2 ++
> > > > > >  1 file changed, 2 insertions(+)
> > > > > >
> > > > >
> > > > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > > > > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> > > > >
> > > > > yamllint warnings/errors:
> > > > >
> > > > > dtschema/dtc warnings/errors:
> > > > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/sff,sfp.yaml: properties:maximum-power-milliwatt: 'minimum' should not be valid under {'enum': ['const', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'minimum', 'maximum', 'multipleOf', 'pattern']}
> > > > >     hint: Scalar and array keywords cannot be mixed
> > > > >     from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
> > > >
> > > > I'm reading that error message and it means absolutely nothing to me.
> > > > Please can you explain it (and also re-word it to be clearer)?
> > >
> > > 'maxItems' is a constraint for arrays. 'maximum' is a constraint for
> > > scalar values. Mixing them does not make sense.
> >
> > TBC, dropping 'maxItems' is what is needed here.
>
> So how does this work?

Do you really want to know? ;)

>
> maxItems: 1

json-schema happily ignores any keywords that it doesn't understand or
don't make sense for a specific context. The DT meta-schema tries to
prevent that.

> tells it that there should be an array of one property, which is at the
> DT level fundamentally the same as a scalar property.

Yes, it is true that the YAML encoded DT and (currently) the internal
encoding used by the tools encode everything as matrices simply
because dtc doing the YAML encoding doesn't know the types beyond what
DTS source level provides, so everything has to be the same encoding.
Now we use the type information in the schemas to decode the DTBs
directly and don't have that limitation. Once I remove the YAML
encoding, we can stop encoding everything as a matrix and having to
fixup the schemas from scalar -> array -> matrix.

> minimum:
> default:
> maximum:
>
> tells it that this is a scalar property, so there should be exactly one
> item or the property should not be mentioned?

Not sure I follow the question. As the property is defined as a
scalar, it only needs scalar keywords. Internally, the schema gets
expanded to:

prop:
  minItems: 1
  maxItems: 1
  items:
    - maxItems: 1
      minItems: 1
      items:
        - maximum: ???
          minimum: ???
          default: ???

This is what processed-schemas.json will contain if you just have the
scalar keywords.

It's a bit more messy now with the unit suffixes as initially they
were all scalars, but over time we've had to allow for arrays. So it's
really they default to scalars unless you need an array in which you
can define:

prop:
  maxItems: 2
  items:
    maximum: ???

Could you do 'maxItems: 1' here? Yes, that would be a valid schema,
but IIRC we'll still complain because it is redundant.

Rob

[PATCH net-next 7/7] net: sfp: get rid of DM7052 hack when enabling high power

[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().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp.c | 29 ++++++-----------------------
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 921bbedd9b22..39fd1811375c 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -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;
 	}
 
-- 
2.30.2

Re: [PATCH net-next 0/7] net: sfp: improve high power module implementation

[Andrew Lunn]

On Wed, Oct 19, 2022 at 02:28:20PM +0100, Russell King (Oracle) 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.
> 
> 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.

Or it is yet another case of violating the standard. The bit is valid,
the revision is wrong in the EEPROM. How long do you think it will be
before we see a quirk like this?

> 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.

Great, the standard itself is broken.

       Andrew

Re: [PATCH net-next 2/7] net: sfp: check firmware provided max power

[Andrew Lunn]

On Wed, Oct 19, 2022 at 02:28:51PM +0100, Russell King (Oracle) wrote:
> Check that the firmware provided maximum power is at least 1W, which
> iis the minimum power level for any SFP module.

is

> 
> 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.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 3/7] net: sfp: ignore power level 2 prior to SFF-8472 Rev 10.2

[Andrew Lunn]

On Wed, Oct 19, 2022 at 02:28:56PM +0100, Russell King (Oracle) wrote:
> 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.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 4/7] net: sfp: ignore power level 3 prior to SFF-8472 Rev 11.4

[Andrew Lunn]

On Wed, Oct 19, 2022 at 02:29:01PM +0100, Russell King (Oracle) wrote:
> 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.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 5/7] net: sfp: provide a definition for the power level select bit

[Andrew Lunn]

On Wed, Oct 19, 2022 at 02:29:06PM +0100, Russell King (Oracle) wrote:
> Provide a named definition for the power level select bit in the
> extended status register, rather than using BIT(0) in the code.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 6/7] net: sfp: add sfp_modify_u8() helper

[Andrew Lunn]

On Wed, Oct 19, 2022 at 02:29:11PM +0100, Russell King (Oracle) wrote:
> 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.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 7/7] net: sfp: get rid of DM7052 hack when enabling high power

[Andrew Lunn]

On Wed, Oct 19, 2022 at 02:29:16PM +0100, Russell King (Oracle) wrote:
> 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().
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 7/7] net: sfp: get rid of DM7052 hack when enabling high power

[Jakub Kicinski]

On Fri, 21 Oct 2022 17:52:52 +0200 Andrew Lunn wrote:
> On Wed, Oct 19, 2022 at 02:29:16PM +0100, Russell King (Oracle) wrote:
> > 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().
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>  
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

FWIW there is a v2 of this, somewhat mis-subjected (pw-bot's
auto-Superseding logic missed it for example):
https://lore.kernel.org/all/E1oltef-00Fwwz-3t@rmk-PC.armlinux.org.uk/

Re: [PATCH net-next 7/7] net: sfp: get rid of DM7052 hack when enabling high power

[Russell King (Oracle)]

On Fri, Oct 21, 2022 at 09:16:18AM -0700, Jakub Kicinski wrote:
> On Fri, 21 Oct 2022 17:52:52 +0200 Andrew Lunn wrote:
> > On Wed, Oct 19, 2022 at 02:29:16PM +0100, Russell King (Oracle) wrote:
> > > 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().
> > > 
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>  
> > 
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> FWIW there is a v2 of this, somewhat mis-subjected (pw-bot's
> auto-Superseding logic missed it for example):
> https://lore.kernel.org/all/E1oltef-00Fwwz-3t@rmk-PC.armlinux.org.uk/

Yes, sadly I forgot to update the subject line prefix. Getting
everything correct every time is quite difficult.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!