[PATCH v2 0/3] net: mdio: mdio-thunder: MDIO clock related changes for Marvell Octeon Family.

[PATCH v2 0/3] net: mdio: mdio-thunder: MDIO clock related changes for Marvell Octeon Family.

[Piyush Malgujar]

This patch series mdio changes are pertaining to Marvell Octeon family.

1) clock gating:
	The purpose of this change is to apply clock gating for MDIO clock
	when there is no transaction happening. This will stop the MDC
	clock toggling in idle scenario.

2) Marvell MDIO clock frequency attribute change:
	This MDIO change provides an option for user to have the bus speed
	set to their needs. The clock-freq for Marvell Octeon defaults to
	3.125 MHz and not 2.5 MHz as standard. In case someone needs to use
	this attribute, they have to add an extra attribute
	"clock-frequency" in the mdio entry in their DTS and this driver
	will do the rest.
        The changes are made in a way that the clock will set to the
	nearest possible value based on the clock calculation and required
	frequency from DTS.
        
These changes has been verified internally with Marvell Octeon series.

Changes since V1:
* Separated the logical changes in separate patches
* Replaced macros with functions
* Used proper property name for DTS
* Updated DTS binding

Piyush Malgujar (3):
  net: mdio: mdio-thunder: stop toggling SMI clock on idle
  dt-bindings: net: cavium-mdio.txt: add clock-frequency attribute
  net: mdio: mdio-thunder: support for clock-freq attribute

 .../devicetree/bindings/net/cavium-mdio.txt   |  5 ++
 drivers/net/mdio/mdio-cavium.h                |  1 +
 drivers/net/mdio/mdio-thunder.c               | 68 +++++++++++++++++++
 3 files changed, 74 insertions(+)

-- 
2.17.1

[PATCH v2 1/3] net: mdio: mdio-thunder: stop toggling SMI clock on idle

[Piyush Malgujar]

SMI clock should be running only for the time when there are transactions
on the bus.

Signed-off-by: Damian Eppel <deppel@marvell.com>
Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
---
 drivers/net/mdio/mdio-thunder.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/mdio/mdio-thunder.c b/drivers/net/mdio/mdio-thunder.c
index 822d2cdd2f3599025f3e79d4243337c18114c951..715c835ace785da345ac037177b0f291678e4c47 100644
--- a/drivers/net/mdio/mdio-thunder.c
+++ b/drivers/net/mdio/mdio-thunder.c
@@ -58,6 +58,7 @@ static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
 		struct resource r;
 		struct mii_bus *mii_bus;
 		struct cavium_mdiobus *bus;
+		union cvmx_smix_clk smi_clk;
 		union cvmx_smix_en smi_en;
 
 		/* If it is not an OF node we cannot handle it yet, so
@@ -87,6 +88,10 @@ static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
 		bus->register_base = nexus->bar0 +
 			r.start - pci_resource_start(pdev, 0);
 
+		smi_clk.u64 = oct_mdio_readq(bus->register_base + SMI_CLK);
+		smi_clk.s.clk_idle = 1;
+		oct_mdio_writeq(smi_clk.u64, bus->register_base + SMI_CLK);
+
 		smi_en.u64 = 0;
 		smi_en.s.en = 1;
 		oct_mdio_writeq(smi_en.u64, bus->register_base + SMI_EN);
-- 
2.17.1

[PATCH v2 2/3] dt-bindings: net: cavium-mdio.txt: add clock-frequency attribute

[Piyush Malgujar]

Add support to configure MDIO clock frequency via DTS

Signed-off-by: Damian Eppel <deppel@marvell.com>
Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
---
 Documentation/devicetree/bindings/net/cavium-mdio.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/cavium-mdio.txt b/Documentation/devicetree/bindings/net/cavium-mdio.txt
index 020df08b8a30f4df80766bb90e100ae6210a777b..638c341966a80823b9eb2f33b947f38110907cc1 100644
--- a/Documentation/devicetree/bindings/net/cavium-mdio.txt
+++ b/Documentation/devicetree/bindings/net/cavium-mdio.txt
@@ -41,6 +41,9 @@ Properties:
 
 - reg: The PCI device and function numbers of the nexus device.
 
+- clock-frequency: MDIO bus clock frequency in Hz. It defaults to 3.125 MHz and
+		   and not to standard 2.5 MHz for Marvell Octeon family.
+
 - #address-cells: Must be <2>.
 
 - #size-cells: Must be <2>.
@@ -64,6 +67,7 @@ Example:
                         #address-cells = <1>;
                         #size-cells = <0>;
                         reg = <0x87e0 0x05003800 0x0 0x30>;
+                        clock-frequency = <3125000>;
 
                         ethernet-phy@0 {
                                 ...
@@ -75,6 +79,7 @@ Example:
                         #address-cells = <1>;
                         #size-cells = <0>;
                         reg = <0x87e0 0x05003880 0x0 0x30>;
+                        clock-frequency = <3125000>;
 
                         ethernet-phy@0 {
                                 ...
-- 
2.17.1

Re: [PATCH v2 2/3] dt-bindings: net: cavium-mdio.txt: add clock-frequency attribute

[Rob Herring]

On Mon, May 30, 2022 at 05:53:27AM -0700, Piyush Malgujar wrote:
> Add support to configure MDIO clock frequency via DTS
> 
> Signed-off-by: Damian Eppel <deppel@marvell.com>
> Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> ---
>  Documentation/devicetree/bindings/net/cavium-mdio.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/cavium-mdio.txt b/Documentation/devicetree/bindings/net/cavium-mdio.txt
> index 020df08b8a30f4df80766bb90e100ae6210a777b..638c341966a80823b9eb2f33b947f38110907cc1 100644
> --- a/Documentation/devicetree/bindings/net/cavium-mdio.txt
> +++ b/Documentation/devicetree/bindings/net/cavium-mdio.txt
> @@ -41,6 +41,9 @@ Properties:
>  
>  - reg: The PCI device and function numbers of the nexus device.
>  
> +- clock-frequency: MDIO bus clock frequency in Hz. It defaults to 3.125 MHz and
> +		   and not to standard 2.5 MHz for Marvell Octeon family.

Already covered by mdio.yaml, so perhaps convert this to DT schema 
format instead.

Rob

Re: [PATCH v2 2/3] dt-bindings: net: cavium-mdio.txt: add clock-frequency attribute

[Andrew Lunn]

On Thu, Jun 02, 2022 at 10:07:55AM -0500, Rob Herring wrote:
> On Mon, May 30, 2022 at 05:53:27AM -0700, Piyush Malgujar wrote:
> > Add support to configure MDIO clock frequency via DTS
> > 
> > Signed-off-by: Damian Eppel <deppel@marvell.com>
> > Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> > ---
> >  Documentation/devicetree/bindings/net/cavium-mdio.txt | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/cavium-mdio.txt b/Documentation/devicetree/bindings/net/cavium-mdio.txt
> > index 020df08b8a30f4df80766bb90e100ae6210a777b..638c341966a80823b9eb2f33b947f38110907cc1 100644
> > --- a/Documentation/devicetree/bindings/net/cavium-mdio.txt
> > +++ b/Documentation/devicetree/bindings/net/cavium-mdio.txt
> > @@ -41,6 +41,9 @@ Properties:
> >  
> >  - reg: The PCI device and function numbers of the nexus device.
> >  
> > +- clock-frequency: MDIO bus clock frequency in Hz. It defaults to 3.125 MHz and
> > +		   and not to standard 2.5 MHz for Marvell Octeon family.

Hi Piyush

There is an ambiguity here in the English. It could be interpreted
that 2.5MHz is the standard for Marvell Octeon family. When in fact
802.3 c22 says it should be up to 2.5MHz.

    For Marvell Octeon family it defaults to 3.125 MHz and not to
    the 802.3 standard 2.5 MHz.

> 
> Already covered by mdio.yaml, so perhaps convert this to DT schema 
> format instead.

Hi Rob

Yes, this property is in mdio.yaml:

  clock-frequency:
    description:
      Desired MDIO bus clock frequency in Hz. Values greater than IEEE 802.3
      defined 2.5MHz should only be used when all devices on the bus support
      the given clock speed.

However, for some reason, this driver decides to break the standard
and defaults to 3.125MHz not 2.5MHz. So i would like that clearly
documented in the binding.

	   Andrew

[PATCH v2 3/3] net: mdio: mdio-thunder: support for clock-freq attribute

[Piyush Malgujar]

Added functionality to modify clock frequency via DTS entry.

Signed-off-by: Damian Eppel <deppel@marvell.com>
Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
---
 drivers/net/mdio/mdio-cavium.h  |  1 +
 drivers/net/mdio/mdio-thunder.c | 63 +++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/drivers/net/mdio/mdio-cavium.h b/drivers/net/mdio/mdio-cavium.h
index a2245d436f5dae4d6424b7c7bfca0aa969a3b3ad..ed4c48d8a38bd80e6a169f7a6d90c1f2a0daccfc 100644
--- a/drivers/net/mdio/mdio-cavium.h
+++ b/drivers/net/mdio/mdio-cavium.h
@@ -92,6 +92,7 @@ struct cavium_mdiobus {
 	struct mii_bus *mii_bus;
 	void __iomem *register_base;
 	enum cavium_mdiobus_mode mode;
+	u32 clk_freq;
 };
 
 #ifdef CONFIG_CAVIUM_OCTEON_SOC
diff --git a/drivers/net/mdio/mdio-thunder.c b/drivers/net/mdio/mdio-thunder.c
index 715c835ace785da345ac037177b0f291678e4c47..7ea6ef0a23f3f5d7df76e3a7aed007ed847f9140 100644
--- a/drivers/net/mdio/mdio-thunder.c
+++ b/drivers/net/mdio/mdio-thunder.c
@@ -14,11 +14,56 @@
 
 #include "mdio-cavium.h"
 
+#define PHASE_MIN 3
+#define PHASE_DFLT 16
+
 struct thunder_mdiobus_nexus {
 	void __iomem *bar0;
 	struct cavium_mdiobus *buses[4];
 };
 
+static inline u32 clk_freq(u32 phase)
+{
+	return (100000000U / (2 * (phase)));
+}
+
+static inline u32 calc_sample(u32 phase)
+{
+	return (2 * (phase) - 3);
+}
+
+static u32 _config_clk(u32 req_freq, u32 *phase, u32 *sample)
+{
+	unsigned int p;
+	u32 freq = 0, freq_prev;
+
+	for (p = PHASE_MIN; p < PHASE_DFLT; p++) {
+		freq_prev = freq;
+		freq = clk_freq(p);
+
+		if (req_freq >= freq)
+			break;
+	}
+
+	if (p == PHASE_DFLT)
+		freq = clk_freq(PHASE_DFLT);
+
+	if (p == PHASE_MIN || p == PHASE_DFLT)
+		goto out;
+
+	/* Check which clock value from the identified range
+	 * is closer to the requested value
+	 */
+	if ((freq_prev - req_freq) < (req_freq - freq)) {
+		p = p - 1;
+		freq = freq_prev;
+	}
+out:
+	*phase = p;
+	*sample = calc_sample(p);
+	return freq;
+}
+
 static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
 				     const struct pci_device_id *ent)
 {
@@ -56,6 +101,7 @@ static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
 	i = 0;
 	device_for_each_child_node(&pdev->dev, fwn) {
 		struct resource r;
+		u32 req_clk_freq;
 		struct mii_bus *mii_bus;
 		struct cavium_mdiobus *bus;
 		union cvmx_smix_clk smi_clk;
@@ -90,6 +136,23 @@ static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
 
 		smi_clk.u64 = oct_mdio_readq(bus->register_base + SMI_CLK);
 		smi_clk.s.clk_idle = 1;
+
+		if (!of_property_read_u32(node, "clock-frequency", &req_clk_freq)) {
+			u32 phase, sample;
+
+			dev_dbg(&pdev->dev, "requested bus clock frequency=%d\n",
+				req_clk_freq);
+
+			bus->clk_freq = _config_clk(req_clk_freq,
+						    &phase, &sample);
+
+			smi_clk.s.phase = phase;
+			smi_clk.s.sample_hi = (sample >> 4) & 0x1f;
+			smi_clk.s.sample = sample & 0xf;
+		} else {
+			bus->clk_freq = clk_freq(PHASE_DFLT);
+		}
+
 		oct_mdio_writeq(smi_clk.u64, bus->register_base + SMI_CLK);
 
 		smi_en.u64 = 0;
-- 
2.17.1

Re: [PATCH v2 3/3] net: mdio: mdio-thunder: support for clock-freq attribute

[Jakub Kicinski]

On Mon, 30 May 2022 05:53:28 -0700 Piyush Malgujar wrote:
> +static inline u32 clk_freq(u32 phase)
> +{
> +	return (100000000U / (2 * (phase)));
> +}
> +
> +static inline u32 calc_sample(u32 phase)

Please drop the inline keyword, the compiler will inline this anyway
and static inline prevents unused code warnings.

> +{
> +	return (2 * (phase) - 3);

No need to wrap arguments to a static inline in brackets.

> +}
> +
> +static u32 _config_clk(u32 req_freq, u32 *phase, u32 *sample)
> +{
> +	unsigned int p;
> +	u32 freq = 0, freq_prev;

It's customary in networking to order variable decl lines longest
to shortest.

> +	for (p = PHASE_MIN; p < PHASE_DFLT; p++) {
> +		freq_prev = freq;
> +		freq = clk_freq(p);
> +
> +		if (req_freq >= freq)
> +			break;
> +	}
> +
> +	if (p == PHASE_DFLT)
> +		freq = clk_freq(PHASE_DFLT);
> +
> +	if (p == PHASE_MIN || p == PHASE_DFLT)
> +		goto out;
> +
> +	/* Check which clock value from the identified range
> +	 * is closer to the requested value
> +	 */
> +	if ((freq_prev - req_freq) < (req_freq - freq)) {

No need for brackets around the arithmetic, assume basic operator
precedence is not broken in the compiler...

Re: [PATCH v2 3/3] net: mdio: mdio-thunder: support for clock-freq attribute

[Andrew Lunn]

> +static inline u32 clk_freq(u32 phase)

Please keep with the naming scheme in the rest of the driver, 
thunder_mdiobus_clk_freq()

> +{
> +	return (100000000U / (2 * (phase)));
> +}
> +
> +static inline u32 calc_sample(u32 phase)
> +{

thunder_mdiobus_calc_sample()

> +	return (2 * (phase) - 3);
> +}
> +
> +static u32 _config_clk(u32 req_freq, u32 *phase, u32 *sample)

thunder_mdiobus_config_clk().

> +{
> +	unsigned int p;
> +	u32 freq = 0, freq_prev;
> +
> +	for (p = PHASE_MIN; p < PHASE_DFLT; p++) {
> +		freq_prev = freq;
> +		freq = clk_freq(p);
> +
> +		if (req_freq >= freq)
> +			break;
> +	}
> +
> +	if (p == PHASE_DFLT)
> +		freq = clk_freq(PHASE_DFLT);
> +
> +	if (p == PHASE_MIN || p == PHASE_DFLT)
> +		goto out;
> +
> +	/* Check which clock value from the identified range
> +	 * is closer to the requested value
> +	 */
> +	if ((freq_prev - req_freq) < (req_freq - freq)) {
> +		p = p - 1;
> +		freq = freq_prev;
> +	}
> +out:
> +	*phase = p;
> +	*sample = calc_sample(p);
> +	return freq;
> +}
> +
>  static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
>  				     const struct pci_device_id *ent)
>  {
> @@ -56,6 +101,7 @@ static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
>  	i = 0;
>  	device_for_each_child_node(&pdev->dev, fwn) {
>  		struct resource r;
> +		u32 req_clk_freq;
>  		struct mii_bus *mii_bus;
>  		struct cavium_mdiobus *bus;
>  		union cvmx_smix_clk smi_clk;
> @@ -90,6 +136,23 @@ static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
>  
>  		smi_clk.u64 = oct_mdio_readq(bus->register_base + SMI_CLK);
>  		smi_clk.s.clk_idle = 1;
> +
> +		if (!of_property_read_u32(node, "clock-frequency", &req_clk_freq)) {
> +			u32 phase, sample;
> +
> +			dev_dbg(&pdev->dev, "requested bus clock frequency=%d\n",
> +				req_clk_freq);
> +
> +			bus->clk_freq = _config_clk(req_clk_freq,
> +						    &phase, &sample);
> +
> +			smi_clk.s.phase = phase;
> +			smi_clk.s.sample_hi = (sample >> 4) & 0x1f;
> +			smi_clk.s.sample = sample & 0xf;
> +		} else {
> +			bus->clk_freq = clk_freq(PHASE_DFLT);
> +		}

You can make this simpler by setting req_clk_freq to your odd
default. Then call of_property_read_u32(). If the property is not
defined, the value of req_clk_freq will not be changed, and the
calculation should come out with the correct value.

	    Andrew

Re: [PATCH v2 0/3] net: mdio: mdio-thunder: MDIO clock related changes for Marvell Octeon Family.

[Jakub Kicinski]

On Mon, 30 May 2022 05:53:25 -0700 Piyush Malgujar wrote:
> This patch series mdio changes are pertaining to Marvell Octeon family.
> 
> 1) clock gating:
> 	The purpose of this change is to apply clock gating for MDIO clock
> 	when there is no transaction happening. This will stop the MDC
> 	clock toggling in idle scenario.
> 
> 2) Marvell MDIO clock frequency attribute change:
> 	This MDIO change provides an option for user to have the bus speed
> 	set to their needs. The clock-freq for Marvell Octeon defaults to
> 	3.125 MHz and not 2.5 MHz as standard. In case someone needs to use
> 	this attribute, they have to add an extra attribute
> 	"clock-frequency" in the mdio entry in their DTS and this driver
> 	will do the rest.
>         The changes are made in a way that the clock will set to the
> 	nearest possible value based on the clock calculation and required
> 	frequency from DTS.
>         
> These changes has been verified internally with Marvell Octeon series.

Thanks for the patches, this does not sound like a fix tho and we're in
the middle of a merge window, so please repost on/after Monday.