[PATCH RFC net-next 1/3] dt-bindings: net: convert mscc-miim to YAML format

[PATCH RFC net-next 1/3] dt-bindings: net: convert mscc-miim to YAML format

[Michael Walle]

Convert the mscc-miim device tree binding to the new YAML format.

The original binding don't mention if the interrupt property is optional
or not. But on the SparX-5 SoC, for example, the interrupt property isn't
used, thus in the new binding that property is optional. FWIW the driver
doesn't use interrupts at all.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 .../devicetree/bindings/net/mscc,miim.yaml    | 55 +++++++++++++++++++
 .../devicetree/bindings/net/mscc-miim.txt     | 26 ---------
 2 files changed, 55 insertions(+), 26 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/mscc,miim.yaml
 delete mode 100644 Documentation/devicetree/bindings/net/mscc-miim.txt

diff --git a/Documentation/devicetree/bindings/net/mscc,miim.yaml b/Documentation/devicetree/bindings/net/mscc,miim.yaml
new file mode 100644
index 000000000000..b52bf1732755
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mscc,miim.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/mscc,miim.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microsemi MII Management Controller (MIIM)
+
+maintainers:
+  - Alexandre Belloni <alexandre.belloni@bootlin.com>
+
+allOf:
+  - $ref: "mdio.yaml#"
+
+properties:
+  compatible:
+    enum:
+      - mscc,ocelot-miim
+      - microchip,lan966x-miim
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  reg:
+    items:
+      - description: base address
+      - description: associated reset register for internal PHYs
+    minItems: 1
+
+  interrupts: true
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    mdio@107009c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      compatible = "mscc,ocelot-miim";
+      reg = <0x107009c 0x36>, <0x10700f0 0x8>;
+      interrupts = <14>;
+
+      phy0: ethernet-phy@0 {
+        reg = <0>;
+      };
+    };
diff --git a/Documentation/devicetree/bindings/net/mscc-miim.txt b/Documentation/devicetree/bindings/net/mscc-miim.txt
deleted file mode 100644
index 70e0cb1ee485..000000000000
--- a/Documentation/devicetree/bindings/net/mscc-miim.txt
+++ /dev/null
@@ -1,26 +0,0 @@
-Microsemi MII Management Controller (MIIM) / MDIO
-=================================================
-
-Properties:
-- compatible: must be "mscc,ocelot-miim" or "microchip,lan966x-miim"
-- reg: The base address of the MDIO bus controller register bank. Optionally, a
-  second register bank can be defined if there is an associated reset register
-  for internal PHYs
-- #address-cells: Must be <1>.
-- #size-cells: Must be <0>.  MDIO addresses have no size component.
-- interrupts: interrupt specifier (refer to the interrupt binding)
-
-Typically an MDIO bus might have several children.
-
-Example:
-	mdio@107009c {
-		#address-cells = <1>;
-		#size-cells = <0>;
-		compatible = "mscc,ocelot-miim";
-		reg = <0x107009c 0x36>, <0x10700f0 0x8>;
-		interrupts = <14>;
-
-		phy0: ethernet-phy@0 {
-			reg = <0>;
-		};
-	};
-- 
2.30.2

[PATCH RFC net-next 2/3] dt-bindings: net: mscc-miim: add clock and clock-frequency

[Michael Walle]

Add the (optional) clock input of the MDIO controller and indicate that
the common clock-frequency property is supported. The driver can use it
to set the desired MDIO bus frequency.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 Documentation/devicetree/bindings/net/mscc,miim.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/mscc,miim.yaml b/Documentation/devicetree/bindings/net/mscc,miim.yaml
index b52bf1732755..e9e8ddcdade9 100644
--- a/Documentation/devicetree/bindings/net/mscc,miim.yaml
+++ b/Documentation/devicetree/bindings/net/mscc,miim.yaml
@@ -32,6 +32,11 @@ properties:
 
   interrupts: true
 
+  clocks:
+    maxItems: 1
+
+  clock-frequency: true
+
 required:
   - compatible
   - reg
-- 
2.30.2

Re: [PATCH RFC net-next 2/3] dt-bindings: net: mscc-miim: add clock and clock-frequency

[Krzysztof Kozlowski]

On 31/03/2022 17:14, Michael Walle wrote:
> Add the (optional) clock input of the MDIO controller and indicate that
> the common clock-frequency property is supported. The driver can use it
> to set the desired MDIO bus frequency.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  Documentation/devicetree/bindings/net/mscc,miim.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/mscc,miim.yaml b/Documentation/devicetree/bindings/net/mscc,miim.yaml
> index b52bf1732755..e9e8ddcdade9 100644
> --- a/Documentation/devicetree/bindings/net/mscc,miim.yaml
> +++ b/Documentation/devicetree/bindings/net/mscc,miim.yaml
> @@ -32,6 +32,11 @@ properties:
>  
>    interrupts: true
>  
> +  clocks:
> +    maxItems: 1
> +
> +  clock-frequency: true

This looks unusual clock-frequency is usually for clock providers but
this is a consumer, so it is not a common frequency here. You mention
that "driver can use it", so it's not a hardware description but some
feature for the driver. We have this already - use assigned-clock* in
your DTS.

Best regards,
Krzysztof

Re: [PATCH RFC net-next 2/3] dt-bindings: net: mscc-miim: add clock and clock-frequency

[Andrew Lunn]

On Thu, Mar 31, 2022 at 10:05:48PM +0200, Krzysztof Kozlowski wrote:
> On 31/03/2022 17:14, Michael Walle wrote:
> > Add the (optional) clock input of the MDIO controller and indicate that
> > the common clock-frequency property is supported. The driver can use it
> > to set the desired MDIO bus frequency.
> > 
> > Signed-off-by: Michael Walle <michael@walle.cc>
> > ---
> >  Documentation/devicetree/bindings/net/mscc,miim.yaml | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/mscc,miim.yaml b/Documentation/devicetree/bindings/net/mscc,miim.yaml
> > index b52bf1732755..e9e8ddcdade9 100644
> > --- a/Documentation/devicetree/bindings/net/mscc,miim.yaml
> > +++ b/Documentation/devicetree/bindings/net/mscc,miim.yaml
> > @@ -32,6 +32,11 @@ properties:
> >  
> >    interrupts: true
> >  
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-frequency: true
> 
> This looks unusual clock-frequency is usually for clock providers but
> this is a consumer, so it is not a common frequency here. You mention
> that "driver can use it", so it's not a hardware description but some
> feature for the driver. We have this already - use assigned-clock* in
> your DTS.

Please see

Documentation/devicetree/bindings/net/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.

The MDIO bus master provides the MDIO bus clock, so in a sense, the
device is a provider. although it does also make use of the clock
itself. It is a hardware description, because the users of the bus
make use of the clock, i.e. the PHY devices on the bus.

It is also identical to i2c bus masters
Documentation/devicetree/bindings/i2c/i2c.txt says:

- clock-frequency
        frequency of bus clock in Hz.

   Andrew

Re: [PATCH RFC net-next 2/3] dt-bindings: net: mscc-miim: add clock and clock-frequency

[Krzysztof Kozlowski]

On 31/03/2022 22:16, Andrew Lunn wrote:
> On Thu, Mar 31, 2022 at 10:05:48PM +0200, Krzysztof Kozlowski wrote:
>> On 31/03/2022 17:14, Michael Walle wrote:
>>> Add the (optional) clock input of the MDIO controller and indicate that
>>> the common clock-frequency property is supported. The driver can use it
>>> to set the desired MDIO bus frequency.
>>>
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> ---
>>>  Documentation/devicetree/bindings/net/mscc,miim.yaml | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/mscc,miim.yaml b/Documentation/devicetree/bindings/net/mscc,miim.yaml
>>> index b52bf1732755..e9e8ddcdade9 100644
>>> --- a/Documentation/devicetree/bindings/net/mscc,miim.yaml
>>> +++ b/Documentation/devicetree/bindings/net/mscc,miim.yaml
>>> @@ -32,6 +32,11 @@ properties:
>>>  
>>>    interrupts: true
>>>  
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  clock-frequency: true
>>
>> This looks unusual clock-frequency is usually for clock providers but
>> this is a consumer, so it is not a common frequency here. You mention
>> that "driver can use it", so it's not a hardware description but some
>> feature for the driver. We have this already - use assigned-clock* in
>> your DTS.
> 
> Please see
> 
> Documentation/devicetree/bindings/net/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.
> 
> The MDIO bus master provides the MDIO bus clock, so in a sense, the
> device is a provider. although it does also make use of the clock
> itself. It is a hardware description, because the users of the bus
> make use of the clock, i.e. the PHY devices on the bus.
> 
> It is also identical to i2c bus masters
> Documentation/devicetree/bindings/i2c/i2c.txt says:
> 

Thanks, it's good. Indeed buses also use this property.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH RFC net-next 3/3] net: phy: mscc-miim: add support to set MDIO bus frequency

[Michael Walle]

Until now, the MDIO bus will have the hardware default bus frequency.
Read the desired frequency of the bus from the device tree and configure
it.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/mdio/mdio-mscc-miim.c | 52 +++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index c9efcfa2a1ce..3793c154bd0d 100644
--- a/drivers/net/mdio/mdio-mscc-miim.c
+++ b/drivers/net/mdio/mdio-mscc-miim.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
@@ -30,6 +31,8 @@
 #define		MSCC_MIIM_CMD_VLD		BIT(31)
 #define MSCC_MIIM_REG_DATA		0xC
 #define		MSCC_MIIM_DATA_ERROR		(BIT(16) | BIT(17))
+#define MSCC_MIIM_REG_CFG		0x10
+#define		MSCC_MIIM_CFG_PRESCALE_MASK	GENMASK(7, 0)
 
 #define MSCC_PHY_REG_PHY_CFG	0x0
 #define		PHY_CFG_PHY_ENA		(BIT(0) | BIT(1) | BIT(2) | BIT(3))
@@ -50,6 +53,8 @@ struct mscc_miim_dev {
 	int mii_status_offset;
 	struct regmap *phy_regs;
 	const struct mscc_miim_info *info;
+	struct clk *clk;
+	u32 clk_freq;
 };
 
 /* When high resolution timers aren't built-in: we can't use usleep_range() as
@@ -242,9 +247,32 @@ int mscc_miim_setup(struct device *dev, struct mii_bus **pbus, const char *name,
 }
 EXPORT_SYMBOL(mscc_miim_setup);
 
+static int mscc_miim_clk_set(struct mii_bus *bus)
+{
+	struct mscc_miim_dev *miim = bus->priv;
+	unsigned long rate;
+	u32 div;
+
+	/* Keep the current settings */
+	if (!miim->clk || !miim->clk_freq)
+		return 0;
+
+	rate = clk_get_rate(miim->clk);
+
+	div = DIV_ROUND_UP(rate, 2 * miim->clk_freq) - 1;
+	if (div == 0 || div & ~MSCC_MIIM_CFG_PRESCALE_MASK) {
+		dev_err(&bus->dev, "Incorrect MDIO clock frequency\n");
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(miim->regs, MSCC_MIIM_REG_CFG,
+				  MSCC_MIIM_CFG_PRESCALE_MASK, div);
+}
+
 static int mscc_miim_probe(struct platform_device *pdev)
 {
 	struct regmap *mii_regmap, *phy_regmap = NULL;
+	struct device_node *np = pdev->dev.of_node;
 	void __iomem *regs, *phy_regs;
 	struct mscc_miim_dev *miim;
 	struct resource *res;
@@ -295,21 +323,41 @@ static int mscc_miim_probe(struct platform_device *pdev)
 	if (!miim->info)
 		return -EINVAL;
 
-	ret = of_mdiobus_register(bus, pdev->dev.of_node);
+	miim->clk = devm_clk_get_optional(&pdev->dev, NULL);
+	if (IS_ERR(miim->clk))
+		return PTR_ERR(miim->clk);
+
+	ret = clk_prepare_enable(miim->clk);
+	if (ret)
+		return ret;
+
+	of_property_read_u32(np, "clock-frequency", &miim->clk_freq);
+
+	ret = mscc_miim_clk_set(bus);
+	if (ret)
+		goto out_disable_clk;
+
+	ret = of_mdiobus_register(bus, np);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
-		return ret;
+		goto out_disable_clk;
 	}
 
 	platform_set_drvdata(pdev, bus);
 
 	return 0;
+
+out_disable_clk:
+	clk_disable_unprepare(miim->clk);
+	return ret;
 }
 
 static int mscc_miim_remove(struct platform_device *pdev)
 {
 	struct mii_bus *bus = platform_get_drvdata(pdev);
+	struct mscc_miim_dev *miim = bus->priv;
 
+	clk_disable_unprepare(miim->clk);
 	mdiobus_unregister(bus);
 
 	return 0;
-- 
2.30.2

Re: [PATCH RFC net-next 3/3] net: phy: mscc-miim: add support to set MDIO bus frequency

[Andrew Lunn]

> @@ -295,21 +323,41 @@ static int mscc_miim_probe(struct platform_device *pdev)
>  	if (!miim->info)
>  		return -EINVAL;
>  
> -	ret = of_mdiobus_register(bus, pdev->dev.of_node);
> +	miim->clk = devm_clk_get_optional(&pdev->dev, NULL);
> +	if (IS_ERR(miim->clk))
> +		return PTR_ERR(miim->clk);
> +
> +	ret = clk_prepare_enable(miim->clk);
> +	if (ret)
> +		return ret;
> +
> +	of_property_read_u32(np, "clock-frequency", &miim->clk_freq);

The clock is optional if there is no "clock-frequency" property.  If
the property does exist, the clock should be mandatory. I don't think
it should silently fail setting the bus frequency because the clock is
missing.

	Andrew

Re: [PATCH RFC net-next 3/3] net: phy: mscc-miim: add support to set MDIO bus frequency

[Michael Walle]

Am 2022-03-31 18:28, schrieb Andrew Lunn:
>> @@ -295,21 +323,41 @@ static int mscc_miim_probe(struct 
>> platform_device *pdev)
>>  	if (!miim->info)
>>  		return -EINVAL;
>> 
>> -	ret = of_mdiobus_register(bus, pdev->dev.of_node);
>> +	miim->clk = devm_clk_get_optional(&pdev->dev, NULL);
>> +	if (IS_ERR(miim->clk))
>> +		return PTR_ERR(miim->clk);
>> +
>> +	ret = clk_prepare_enable(miim->clk);
>> +	if (ret)
>> +		return ret;
>> +
>> +	of_property_read_u32(np, "clock-frequency", &miim->clk_freq);
> 
> The clock is optional if there is no "clock-frequency" property.  If
> the property does exist, the clock should be mandatory. I don't think
> it should silently fail setting the bus frequency because the clock is
> missing.

Oh yes, agreed.

-michael

Re: [PATCH RFC net-next 1/3] dt-bindings: net: convert mscc-miim to YAML format

[Michael Walle]

[Forgot the cover letter]

Subject: [PATCH RFC net-next 0/3] net: phy: mscc-miim: add MDIO bus
  frequency support

Introduce MDIO bus frequency support. This way the board can have a
faster (or maybe slower) bus frequency than the hardware default.

Michael Walle (3):
   dt-bindings: net: convert mscc-miim to YAML format
   dt-bindings: net: mscc-miim: add clock and clock-frequency
   net: phy: mscc-miim: add support to set MDIO bus frequency

  .../devicetree/bindings/net/mscc,miim.yaml    | 60 +++++++++++++++++++
  .../devicetree/bindings/net/mscc-miim.txt     | 26 --------
  drivers/net/mdio/mdio-mscc-miim.c             | 52 +++++++++++++++-
  3 files changed, 110 insertions(+), 28 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/net/mscc,miim.yaml
  delete mode 100644 Documentation/devicetree/bindings/net/mscc-miim.txt

-- 
2.30.2

Re: [PATCH RFC net-next 1/3] dt-bindings: net: convert mscc-miim to YAML format

[Krzysztof Kozlowski]

On 31/03/2022 17:14, Michael Walle wrote:
> Convert the mscc-miim device tree binding to the new YAML format.
> 
> The original binding don't mention if the interrupt property is optional
> or not. But on the SparX-5 SoC, for example, the interrupt property isn't
> used, thus in the new binding that property is optional. FWIW the driver
> doesn't use interrupts at all.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  .../devicetree/bindings/net/mscc,miim.yaml    | 55 +++++++++++++++++++
>  .../devicetree/bindings/net/mscc-miim.txt     | 26 ---------
>  2 files changed, 55 insertions(+), 26 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/mscc,miim.yaml
>  delete mode 100644 Documentation/devicetree/bindings/net/mscc-miim.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/mscc,miim.yaml b/Documentation/devicetree/bindings/net/mscc,miim.yaml
> new file mode 100644
> index 000000000000..b52bf1732755
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/mscc,miim.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/mscc,miim.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microsemi MII Management Controller (MIIM)
> +
> +maintainers:
> +  - Alexandre Belloni <alexandre.belloni@bootlin.com>
> +
> +allOf:
> +  - $ref: "mdio.yaml#"
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mscc,ocelot-miim
> +      - microchip,lan966x-miim
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  reg:
> +    items:
> +      - description: base address
> +      - description: associated reset register for internal PHYs
> +    minItems: 1
> +
> +  interrupts: true

how many? maxItems

> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    mdio@107009c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      compatible = "mscc,ocelot-miim";
> +      reg = <0x107009c 0x36>, <0x10700f0 0x8>;

Please put the compatible followed by reg at the beginning (first
properties).

Best regards,
Krzysztof