[PATCH 0/3] adin: add support for 125MHz clk-out

[PATCH 0/3] adin: add support for 125MHz clk-out

[Josua Mayer]

This patch series adds support for enabling a 125MHz reference clock output
to the adin 1300 driver.
Finally this clock output is used to add support for SolidRun i.MX6 SoMs
revision 1.9 and later, which have replaced the original ethernet phy with an
adin 1300.

Josua Mayer (3):
  dt: adin: document clk-out property
  net: phy: adin: add support for 125MHz clk-out
  ARM: dts: imx6qdl-sr-som: update phy configuration for som revision
    1.9

 .../devicetree/bindings/net/adi,adin.yaml     |  5 ++++
 arch/arm/boot/dts/imx6qdl-sr-som.dtsi         |  6 ++++
 drivers/net/phy/adin.c                        | 30 +++++++++++++++++++
 3 files changed, 41 insertions(+)

-- 
2.34.1

[PATCH 1/3] dt: adin: document clk-out property

[Josua Mayer]

The ADIN1300 supports generating certain clocks on its GP_CLK pin.
Add a DT property to specify the frequency.

Due to the complexity of the clock configuration register, for now only
125MHz is documented.

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 Documentation/devicetree/bindings/net/adi,adin.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
index 1129f2b58e98..4e421bf5193d 100644
--- a/Documentation/devicetree/bindings/net/adi,adin.yaml
+++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
@@ -36,6 +36,11 @@ properties:
     enum: [ 4, 8, 12, 16, 20, 24 ]
     default: 8
 
+  adi,clk-out-frequency:
+    description: Clock output frequency in Hertz.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [125000000]
+
 unevaluatedProperties: false
 
 examples:
-- 
2.34.1

[PATCH 2/3] net: phy: adin: add support for 125MHz clk-out

[Josua Mayer]

The ADIN1300 supports generating certain clocks on its GP_CLK pin.
Add support for selecting the 125MHz clock via a device-tree property.

While other frequencies are technically available, they are omitted for
now, due to the complexity of choices.

Co-developed-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Josua Mayer<josua@solid-run.com>
---
 drivers/net/phy/adin.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 5ce6da62cc8e..dbe2bb7f30d9 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -99,6 +99,10 @@
 #define ADIN1300_GE_SOFT_RESET_REG		0xff0c
 #define   ADIN1300_GE_SOFT_RESET		BIT(0)
 
+#define ADIN1300_GE_CLK_CFG_REG			0xff1f
+#define   ADIN1300_GE_CLK_CFG_MASK		GENMASK(5, 0)
+#define   ADIN1300_GE_CLK_CFG_FREE_125		BIT(4)
+
 #define ADIN1300_GE_RGMII_CFG_REG		0xff23
 #define   ADIN1300_GE_RGMII_RX_MSK		GENMASK(8, 6)
 #define   ADIN1300_GE_RGMII_RX_SEL(x)		\
@@ -433,6 +437,28 @@ static int adin_set_tunable(struct phy_device *phydev,
 	}
 }
 
+static int adin_config_clk_out(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	u32 val;
+	u8 sel;
+
+	if (device_property_read_u32(dev, "adi,clk-out-frequency", &val))
+		return 0;
+
+	switch (val) {
+	case 125000000:
+		sel = ADIN1300_GE_CLK_CFG_FREE_125;
+		break;
+	default:
+		phydev_err(phydev, "invalid adi,clk-out-frequency\n");
+		return -EINVAL;
+	}
+
+	return phy_modify_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_CLK_CFG_REG,
+			      ADIN1300_GE_CLK_CFG_MASK, sel);
+}
+
 static int adin_config_init(struct phy_device *phydev)
 {
 	int rc;
@@ -455,6 +481,10 @@ static int adin_config_init(struct phy_device *phydev)
 	if (rc < 0)
 		return rc;
 
+	rc = adin_config_clk_out(phydev);
+	if (rc < 0)
+		return rc;
+
 	phydev_dbg(phydev, "PHY is using mode '%s'\n",
 		   phy_modes(phydev->interface));
 
-- 
2.34.1

[PATCH 3/3] ARM: dts: imx6qdl-sr-som: update phy configuration for som revision 1.9

[Josua Mayer]

Since SoM revision 1.9 the PHY has been replaced with an ADIN1300,
add an entry for it next to the original.

Co-developed-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 arch/arm/boot/dts/imx6qdl-sr-som.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
index f86efd0ccc40..04fd4c02b1c6 100644
--- a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
@@ -83,6 +83,12 @@ ethernet-phy@4 {
 			qca,clk-out-frequency = <125000000>;
 			qca,smarteee-tw-us-1g = <24>;
 		};
+
+		/* ADIN1300 (som rev 1.9 or later) */
+		ethernet-phy@1 {
+			reg = <1>;
+			adi,clk-out-frequency = <125000000>;
+		};
 	};
 };
 
-- 
2.34.1

Re: [PATCH 1/3] dt: adin: document clk-out property

[Krzysztof Kozlowski]

On 10/04/2022 12:46, Josua Mayer wrote:
> The ADIN1300 supports generating certain clocks on its GP_CLK pin.
> Add a DT property to specify the frequency.
> 
> Due to the complexity of the clock configuration register, for now only
> 125MHz is documented.

Thank you for your patch. There is something to discuss/improve.

Adjust subject prefix to the subsystem (dt-bindings, not dt, missing net).

> 
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> ---
>  Documentation/devicetree/bindings/net/adi,adin.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
> index 1129f2b58e98..4e421bf5193d 100644
> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml
> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
> @@ -36,6 +36,11 @@ properties:
>      enum: [ 4, 8, 12, 16, 20, 24 ]
>      default: 8
>  
> +  adi,clk-out-frequency:

Use types defined by the dtschema, so "adi,clk-out-hz". Then no need for
type/ref.

> +    description: Clock output frequency in Hertz.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [125000000]

If only one value, then "const: 125000000", but why do you need such
value in DT if it is always the same?


Best regards,
Krzysztof

Re: [PATCH 1/3] dt: adin: document clk-out property

[Josua Mayer]

\o/

Thank you for the quick reply!

Am 10.04.22 um 17:21 schrieb Krzysztof Kozlowski:
> On 10/04/2022 12:46, Josua Mayer wrote:
>> The ADIN1300 supports generating certain clocks on its GP_CLK pin.
>> Add a DT property to specify the frequency.
>>
>> Due to the complexity of the clock configuration register, for now only
>> 125MHz is documented.
> Thank you for your patch. There is something to discuss/improve.
>
> Adjust subject prefix to the subsystem (dt-bindings, not dt, missing net).
Ack. So something like
dt-bindings: net: adin: document clk-out property
>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>> ---
>>   Documentation/devicetree/bindings/net/adi,adin.yaml | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
>> index 1129f2b58e98..4e421bf5193d 100644
>> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml
>> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
>> @@ -36,6 +36,11 @@ properties:
>>       enum: [ 4, 8, 12, 16, 20, 24 ]
>>       default: 8
>>   
>> +  adi,clk-out-frequency:
> Use types defined by the dtschema, so "adi,clk-out-hz". Then no need for
> type/ref.
That sounds useful, I was not aware. The only inspiration I used was the 
at803x driver ...
It seemed natural to share the property name as it serves the same 
purpose here.
>> +    description: Clock output frequency in Hertz.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [125000000]
> If only one value, then "const: 125000000", but why do you need such
> value in DT if it is always the same?
Yes yes yes.
 From my understanding of the adin1300 data-sheet, it can provide either 
a 25MHz clock or a 125MHz clock on its GP_CLK pin. So for the context of 
this feature we would have two options. However because we found the 
documentation very confusing we skipped the 25MHz option.

Actually my statement above omits some of the options.
- There are actually two 125MHz clocks, the first called "recovered" and 
the second "free running".
- One can let the phy choose the rate based on its internal state.
This is indicated on page 73 of the datasheet
(https://www.analog.com/media/en/technical-documentation/data-sheets/adin1300.pdf)

Because of this confusion we wanted to for now only allow selecting the 
free-running 125MHz clock.

Sincerely
Josua Mayer

Re: [PATCH 1/3] dt: adin: document clk-out property

[Krzysztof Kozlowski]

On 10/04/2022 20:41, Josua Mayer wrote:
>>
>> Adjust subject prefix to the subsystem (dt-bindings, not dt, missing net).
> Ack. So something like
> dt-bindings: net: adin: document clk-out property

Yes.


>>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>>> ---
>>>   Documentation/devicetree/bindings/net/adi,adin.yaml | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
>>> index 1129f2b58e98..4e421bf5193d 100644
>>> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml
>>> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
>>> @@ -36,6 +36,11 @@ properties:
>>>       enum: [ 4, 8, 12, 16, 20, 24 ]
>>>       default: 8
>>>   
>>> +  adi,clk-out-frequency:
>> Use types defined by the dtschema, so "adi,clk-out-hz". Then no need for
>> type/ref.
> That sounds useful, I was not aware. The only inspiration I used was the 
> at803x driver ...
> It seemed natural to share the property name as it serves the same 
> purpose here.

Indeed ar803x uses such property. In general reusing properties is a
good idea, but not all properties are good enough to copy. I don't know
why adi,clk-out-frequency got accepted because we really stick to common
units when possible.

https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

>>> +    description: Clock output frequency in Hertz.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [125000000]
>> If only one value, then "const: 125000000", but why do you need such
>> value in DT if it is always the same?
> Yes yes yes.
>  From my understanding of the adin1300 data-sheet, it can provide either 
> a 25MHz clock or a 125MHz clock on its GP_CLK pin. So for the context of 
> this feature we would have two options. However because we found the 
> documentation very confusing we skipped the 25MHz option.
> 
> Actually my statement above omits some of the options.
> - There are actually two 125MHz clocks, the first called "recovered" and 
> the second "free running".
> - One can let the phy choose the rate based on its internal state.
> This is indicated on page 73 of the datasheet
> (https://www.analog.com/media/en/technical-documentation/data-sheets/adin1300.pdf)
> 
> Because of this confusion we wanted to for now only allow selecting the 
> free-running 125MHz clock.

Hm, so you do not insist on actual frequency but rather type of the
clock (freerunning instead of recovered and 25 MHz). Then the frequency
does not look enough because it does not offer you the choice of clock
(freerunning or recovered) and instead you could have enum like:
  adi,phy-output-clock:
  $ref: /schemas/types.yaml#/definitions/string
  enum: [125mhz-freerunning, 125mhz-recovered, 25mhz-freeruning....]

Judging by page 24 you have 5 or more options... This could be also
numeric ID (enum [0, 1, 2, 3 ...]) with some explanation, but strings
seem easier to understand.

The binding should describe the hardware, not implementation in Linux,
therefore you should actually list all possible choices. The driver then
can just return EINVAL on unsupported choices (or map them back to only
one supported).


Best regards,
Krzysztof

Re: [PATCH 1/3] dt: adin: document clk-out property

[Josua Mayer]

\o/

Am 10.04.22 um 22:01 schrieb Krzysztof Kozlowski:
> On 10/04/2022 20:41, Josua Mayer wrote:
>>> Adjust subject prefix to the subsystem (dt-bindings, not dt, missing net).
>> Ack. So something like
>> dt-bindings: net: adin: document clk-out property
> Yes.
Great, I will have it changed in a future revision!
>>>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/net/adi,adin.yaml | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
>>>> index 1129f2b58e98..4e421bf5193d 100644
>>>> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
>>>> @@ -36,6 +36,11 @@ properties:
>>>>        enum: [ 4, 8, 12, 16, 20, 24 ]
>>>>        default: 8
>>>>    
>>>> +  adi,clk-out-frequency:
>>> Use types defined by the dtschema, so "adi,clk-out-hz". Then no need for
>>> type/ref.
>> That sounds useful, I was not aware. The only inspiration I used was the
>> at803x driver ...
>> It seemed natural to share the property name as it serves the same
>> purpose here.
> Indeed ar803x uses such property. In general reusing properties is a
> good idea, but not all properties are good enough to copy. I don't know
> why adi,clk-out-frequency got accepted because we really stick to common
> units when possible.
>
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
>
>>>> +    description: Clock output frequency in Hertz.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    enum: [125000000]
>>> If only one value, then "const: 125000000", but why do you need such
>>> value in DT if it is always the same?
>> Yes yes yes.
>>   From my understanding of the adin1300 data-sheet, it can provide either
>> a 25MHz clock or a 125MHz clock on its GP_CLK pin. So for the context of
>> this feature we would have two options. However because we found the
>> documentation very confusing we skipped the 25MHz option.
>>
>> Actually my statement above omits some of the options.
>> - There are actually two 125MHz clocks, the first called "recovered" and
>> the second "free running".
>> - One can let the phy choose the rate based on its internal state.
>> This is indicated on page 73 of the datasheet
>> (https://www.analog.com/media/en/technical-documentation/data-sheets/adin1300.pdf)
>>
>> Because of this confusion we wanted to for now only allow selecting the
>> free-running 125MHz clock.
> Hm, so you do not insist on actual frequency but rather type of the
> clock (freerunning instead of recovered and 25 MHz). Then the frequency
> does not look enough because it does not offer you the choice of clock
> (freerunning or recovered) and instead you could have enum like:
>    adi,phy-output-clock:
>    $ref: /schemas/types.yaml#/definitions/string
>    enum: [125mhz-freerunning, 125mhz-recovered, 25mhz-freeruning....]
>
> Judging by page 24 you have 5 or more options... This could be also
> numeric ID (enum [0, 1, 2, 3 ...]) with some explanation, but strings
> seem easier to understand.

I agree that strings are more meaningful here, especially considering 
how each entry carries at least two pieces of information.
If we are not to reuse the qca,clk-out-frequency name, then an enum 
seems the easiest way to describe the available settings from the clock 
config register!

> The binding should describe the hardware, not implementation in Linux,
> therefore you should actually list all possible choices. The driver then
> can just return EINVAL on unsupported choices (or map them back to only
> one supported).

I have prepared a draft for the entries that should exist, it covers 
five of the 6 available bits. Maybe you can comment if this is 
understandable?

   adi,phy-output-clock:
     description: Select clock output on GP_CLK pin. Three clocks are 
available:
       A 25MHz reference, a free-running 125MHz and a recovered 125MHz.
       The phy can also automatically switch between the reference and the
       respective 125MHz clocks based on its internal state.
     $ref: /schemas/types.yaml#/definitions/string
     enum:
     - 25mhz-reference
     - 125mhz-free-running
     - 125mhz-recovered
     - adaptive-free-running
     - adaptive-recovered

Bit no. 3 (GE_REF_CLK_EN) is special in that it can be enabled 
independently from the 5 choices above,
and it controls a different pin. Therefore it deserves its own property, 
perhaps a flag or boolean adi,phy-output-ref-clock.
Any opinion if this should be added, or we can omit it completely?

sincerely
Josua Mayer

Re: [PATCH 1/3] dt: adin: document clk-out property

[Jakub Kicinski]

On Mon, 11 Apr 2022 10:42:18 +0300 Josua Mayer wrote:
> > The binding should describe the hardware, not implementation in Linux,
> > therefore you should actually list all possible choices. The driver then
> > can just return EINVAL on unsupported choices (or map them back to only
> > one supported).  
> 
> I have prepared a draft for the entries that should exist, it covers 
> five of the 6 available bits. Maybe you can comment if this is 
> understandable?
> 
>    adi,phy-output-clock:
>      description: Select clock output on GP_CLK pin. Three clocks are 
> available:
>        A 25MHz reference, a free-running 125MHz and a recovered 125MHz.
>        The phy can also automatically switch between the reference and the
>        respective 125MHz clocks based on its internal state.
>      $ref: /schemas/types.yaml#/definitions/string
>      enum:
>      - 25mhz-reference
>      - 125mhz-free-running
>      - 125mhz-recovered
>      - adaptive-free-running
>      - adaptive-recovered
> 
> Bit no. 3 (GE_REF_CLK_EN) is special in that it can be enabled 
> independently from the 5 choices above,
> and it controls a different pin. Therefore it deserves its own property, 
> perhaps a flag or boolean adi,phy-output-ref-clock.
> Any opinion if this should be added, or we can omit it completely?

Noob question - can you explain how this property describes HW?
I thought we had a framework for clock config, and did not require
vendor specific properties of this sort.

The recovered vs free running makes the entire thing sound like
a SyncE related knob, irrelevant to normal HW operation.

Re: [PATCH 1/3] dt: adin: document clk-out property

[Andrew Lunn]

> Noob question - can you explain how this property describes HW?
> I thought we had a framework for clock config, and did not require
> vendor specific properties of this sort.
> 
> The recovered vs free running makes the entire thing sound like
> a SyncE related knob, irrelevant to normal HW operation.

It is not necessarily SyncE. Fast Ethernet is based around a 25MHz
clock. Something needs to provide that clock. Sometimes the SoC/MAC
provides it, and passes it to the PHY. Sometimes the PHY provides it,
and passes it to the SoC/MAC.

There are a couple of PHYs which make use of the common clock
framework, when the SoC is the clock source. However, i don't think
there are any PHYs which provide a clock to the common clock framework
when they are the source. We do however have a number of vendor
properties to control the PHY clock output, disable the PHY clock
output, select the PHY clock output, etc. There is not too much
standardisation here, and it is made worse by some PHYs needing a
reset once the clock is ticking, some MACs stop the clock when the
link is administrative down, some PHYs stop the clock a short time
after the link goes down which can be bad for the MAC etc.

      Andrew

Re: [PATCH 1/3] dt: adin: document clk-out property

[Jakub Kicinski]

On Mon, 11 Apr 2022 22:59:59 +0200 Andrew Lunn wrote:
> > Noob question - can you explain how this property describes HW?
> > I thought we had a framework for clock config, and did not require
> > vendor specific properties of this sort.
> > 
> > The recovered vs free running makes the entire thing sound like
> > a SyncE related knob, irrelevant to normal HW operation.  
> 
> It is not necessarily SyncE. Fast Ethernet is based around a 25MHz
> clock. Something needs to provide that clock. Sometimes the SoC/MAC
> provides it, and passes it to the PHY. Sometimes the PHY provides it,
> and passes it to the SoC/MAC.
> 
> There are a couple of PHYs which make use of the common clock
> framework, when the SoC is the clock source. However, i don't think
> there are any PHYs which provide a clock to the common clock framework
> when they are the source. We do however have a number of vendor
> properties to control the PHY clock output, disable the PHY clock
> output, select the PHY clock output, etc. There is not too much
> standardisation here, and it is made worse by some PHYs needing a
> reset once the clock is ticking, some MACs stop the clock when the
> link is administrative down, some PHYs stop the clock a short time
> after the link goes down which can be bad for the MAC etc.

I see. Why would the MAC/SoC care if the clock is recovered or free
running, tho?

Re: [PATCH 1/3] dt: adin: document clk-out property

[Andrew Lunn]

On Mon, Apr 11, 2022 at 02:33:15PM -0700, Jakub Kicinski wrote:
> On Mon, 11 Apr 2022 22:59:59 +0200 Andrew Lunn wrote:
> > > Noob question - can you explain how this property describes HW?
> > > I thought we had a framework for clock config, and did not require
> > > vendor specific properties of this sort.
> > > 
> > > The recovered vs free running makes the entire thing sound like
> > > a SyncE related knob, irrelevant to normal HW operation.  
> > 
> > It is not necessarily SyncE. Fast Ethernet is based around a 25MHz
> > clock. Something needs to provide that clock. Sometimes the SoC/MAC
> > provides it, and passes it to the PHY. Sometimes the PHY provides it,
> > and passes it to the SoC/MAC.
> > 
> > There are a couple of PHYs which make use of the common clock
> > framework, when the SoC is the clock source. However, i don't think
> > there are any PHYs which provide a clock to the common clock framework
> > when they are the source. We do however have a number of vendor
> > properties to control the PHY clock output, disable the PHY clock
> > output, select the PHY clock output, etc. There is not too much
> > standardisation here, and it is made worse by some PHYs needing a
> > reset once the clock is ticking, some MACs stop the clock when the
> > link is administrative down, some PHYs stop the clock a short time
> > after the link goes down which can be bad for the MAC etc.
> 
> I see. Why would the MAC/SoC care if the clock is recovered or free
> running, tho?

Autoneg determines which link peer will be the master and which will
be the slave. The master provides the clock for the link. So the slave
PHY might want to pass through the recovered clock so it does not need
to deal with drift between the recovered clock and its own clock.

   Andrew

[PATCH v2 0/3] adin: add support for clock output

[Josua Mayer]

This patch series adds support for configuring the two clock outputs of adin
1200 and 1300 PHYs. Certain network controllers require an external reference
clock which can be provided by the PHY.

One of the replies to v1 was asking why the common clock framework isn't used.
Currently no PHY driver has implemented providing a clock to the network
controller. Instead they rely on vendor extensions to make the appropriate
configuration. For example ar8035 uses qca,clk-out-frequency - this patchset
aimed to replicate the same functionality.

Finally the 125MHz free-running clock is enabled in the device-tree for
SolidRun i.MX6 SoMs, to support revisions 1.9 and later, where the original phy
has been replaced with an adin 1300.

Changes since v1:
- renamed device-tree property and changed to enum
- added device-tree property for second clock output
- implemented all bits from the clock configuration register

Josua Mayer (3):
  dt-bindings: net: adin: document phy clock output properties
  net: phy: adin: add support for clock output
  ARM: dts: imx6qdl-sr-som: update phy configuration for som revision
    1.9

 .../devicetree/bindings/net/adi,adin.yaml     | 17 +++++++
 arch/arm/boot/dts/imx6qdl-sr-som.dtsi         |  6 +++
 drivers/net/phy/adin.c                        | 44 +++++++++++++++++++
 3 files changed, 67 insertions(+)

-- 
2.34.1

[PATCH v2 1/3] dt-bindings: net: adin: document phy clock output properties

[Josua Mayer]

The ADIN1300 supports generating certain clocks on its GP_CLK pin, as
well as providing the reference clock on CLK25_REF.

Add DT properties to configure both pins.

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
V1 -> V2: changed clkout property to enum
V1 -> V2: added property for CLK25_REF pin

 .../devicetree/bindings/net/adi,adin.yaml       | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
index 1129f2b58e98..3e0c6304f190 100644
--- a/Documentation/devicetree/bindings/net/adi,adin.yaml
+++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
@@ -36,6 +36,23 @@ properties:
     enum: [ 4, 8, 12, 16, 20, 24 ]
     default: 8
 
+  adi,phy-output-clock:
+    description: Select clock output on GP_CLK pin. Three clocks are available:
+      A 25MHz reference, a free-running 125MHz and a recovered 125MHz.
+      The phy can also automatically switch between the reference and the
+      respective 125MHz clocks based on its internal state.
+    $ref: /schemas/types.yaml#/definitions/string
+    enum:
+    - 25mhz-reference
+    - 125mhz-free-running
+    - 125mhz-recovered
+    - adaptive-free-running
+    - adaptive-recovered
+
+  adi,phy-output-reference-clock:
+    description: Enable 25MHz reference clock output on CLK25_REF pin.
+    $ref: /schemas/types.yaml#/definitions/flag
+
 unevaluatedProperties: false
 
 examples:
-- 
2.34.1

[PATCH v2 2/3] net: phy: adin: add support for clock output

[Josua Mayer]

The ADIN1300 supports generating certain clocks on its GP_CLK pin, as
well as providing the reference clock on CLK25_REF.

Add support for selecting the clock via device-tree properties.

Co-developed-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Josua Mayer<josua@solid-run.com>
---
V1 -> V2: revised dts property name for clock(s)
V1 -> V2: implemented all 6 bits in the clock configuration register

 drivers/net/phy/adin.c | 44 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 5ce6da62cc8e..e7150a8e34d2 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -99,6 +99,15 @@
 #define ADIN1300_GE_SOFT_RESET_REG		0xff0c
 #define   ADIN1300_GE_SOFT_RESET		BIT(0)
 
+#define ADIN1300_GE_CLK_CFG_REG			0xff1f
+#define   ADIN1300_GE_CLK_CFG_MASK		GENMASK(5, 0)
+#define   ADIN1300_GE_CLK_CFG_RCVR_125		BIT(5)
+#define   ADIN1300_GE_CLK_CFG_FREE_125		BIT(4)
+#define   ADIN1300_GE_CLK_CFG_REF_EN		BIT(3)
+#define   ADIN1300_GE_CLK_CFG_HRT_RCVR		BIT(2)
+#define   ADIN1300_GE_CLK_CFG_HRT_FREE		BIT(1)
+#define   ADIN1300_GE_CLK_CFG_25		BIT(0)
+
 #define ADIN1300_GE_RGMII_CFG_REG		0xff23
 #define   ADIN1300_GE_RGMII_RX_MSK		GENMASK(8, 6)
 #define   ADIN1300_GE_RGMII_RX_SEL(x)		\
@@ -433,6 +442,37 @@ static int adin_set_tunable(struct phy_device *phydev,
 	}
 }
 
+static int adin_config_clk_out(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	const char *val = 0;
+	u8 sel = 0;
+
+	device_property_read_string(dev, "adi,phy-output-clock", &val);
+	if(!val) {
+		/* property not present, do not enable GP_CLK pin */
+	} else if(strcmp(val, "25mhz-reference") == 0) {
+		sel |= ADIN1300_GE_CLK_CFG_25;
+	} else if(strcmp(val, "125mhz-free-running") == 0) {
+		sel |= ADIN1300_GE_CLK_CFG_FREE_125;
+	} else if(strcmp(val, "125mhz-recovered") == 0) {
+		sel |= ADIN1300_GE_CLK_CFG_RCVR_125;
+	} else if(strcmp(val, "adaptive-free-running") == 0) {
+		sel |= ADIN1300_GE_CLK_CFG_HRT_FREE;
+	} else if(strcmp(val, "adaptive-recovered") == 0) {
+		sel |= ADIN1300_GE_CLK_CFG_HRT_RCVR;
+	} else {
+		phydev_err(phydev, "invalid adi,phy-output-clock\n");
+		return -EINVAL;
+	}
+
+	if(device_property_read_bool(dev, "adi,phy-output-reference-clock"))
+		sel |= ADIN1300_GE_CLK_CFG_REF_EN;
+
+	return phy_modify_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_CLK_CFG_REG,
+			      ADIN1300_GE_CLK_CFG_MASK, sel);
+}
+
 static int adin_config_init(struct phy_device *phydev)
 {
 	int rc;
@@ -455,6 +495,10 @@ static int adin_config_init(struct phy_device *phydev)
 	if (rc < 0)
 		return rc;
 
+	rc = adin_config_clk_out(phydev);
+	if (rc < 0)
+		return rc;
+
 	phydev_dbg(phydev, "PHY is using mode '%s'\n",
 		   phy_modes(phydev->interface));
 
-- 
2.34.1

[PATCH v2 3/3] ARM: dts: imx6qdl-sr-som: update phy configuration for som revision 1.9

[Josua Mayer]

Since SoM revision 1.9 the PHY has been replaced with an ADIN1300,
add an entry for it next to the original.

Co-developed-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
V1 -> V2: changed dts property name

 arch/arm/boot/dts/imx6qdl-sr-som.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
index f86efd0ccc40..d46182095d79 100644
--- a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
@@ -83,6 +83,12 @@ ethernet-phy@4 {
 			qca,clk-out-frequency = <125000000>;
 			qca,smarteee-tw-us-1g = <24>;
 		};
+
+		/* ADIN1300 (som rev 1.9 or later) */
+		ethernet-phy@1 {
+			reg = <1>;
+			adi,phy-output-clock = "125mhz-free-running";
+		};
 	};
 };
 
-- 
2.34.1

Re: [PATCH v2 2/3] net: phy: adin: add support for clock output

[kernel test robot]

Hi Josua,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on net/master net-next/master v5.18-rc3 next-20220420]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Josua-Mayer/dt-bindings-net-adin-document-phy-clock-output-properties/20220419-192719
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: openrisc-randconfig-s032-20220420 (https://download.01.org/0day-ci/archive/20220421/202204211324.qgcPMycQ-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 11.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/intel-lab-lkp/linux/commit/74d856f1c89a6534fd58889f20ad4b481b8191c9
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Josua-Mayer/dt-bindings-net-adin-document-phy-clock-output-properties/20220419-192719
        git checkout 74d856f1c89a6534fd58889f20ad4b481b8191c9
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=openrisc SHELL=/bin/bash drivers/net/phy/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/net/phy/adin.c:448:27: sparse: sparse: Using plain integer as NULL pointer

vim +448 drivers/net/phy/adin.c

   444	
   445	static int adin_config_clk_out(struct phy_device *phydev)
   446	{
   447		struct device *dev = &phydev->mdio.dev;
 > 448		const char *val = 0;
   449		u8 sel = 0;
   450	
   451		device_property_read_string(dev, "adi,phy-output-clock", &val);
   452		if(!val) {
   453			/* property not present, do not enable GP_CLK pin */
   454		} else if(strcmp(val, "25mhz-reference") == 0) {
   455			sel |= ADIN1300_GE_CLK_CFG_25;
   456		} else if(strcmp(val, "125mhz-free-running") == 0) {
   457			sel |= ADIN1300_GE_CLK_CFG_FREE_125;
   458		} else if(strcmp(val, "125mhz-recovered") == 0) {
   459			sel |= ADIN1300_GE_CLK_CFG_RCVR_125;
   460		} else if(strcmp(val, "adaptive-free-running") == 0) {
   461			sel |= ADIN1300_GE_CLK_CFG_HRT_FREE;
   462		} else if(strcmp(val, "adaptive-recovered") == 0) {
   463			sel |= ADIN1300_GE_CLK_CFG_HRT_RCVR;
   464		} else {
   465			phydev_err(phydev, "invalid adi,phy-output-clock\n");
   466			return -EINVAL;
   467		}
   468	
   469		if(device_property_read_bool(dev, "adi,phy-output-reference-clock"))
   470			sel |= ADIN1300_GE_CLK_CFG_REF_EN;
   471	
   472		return phy_modify_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_CLK_CFG_REG,
   473				      ADIN1300_GE_CLK_CFG_MASK, sel);
   474	}
   475	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 1/3] dt-bindings: net: adin: document phy clock output properties

[Andrew Lunn]

On Tue, Apr 19, 2022 at 01:27:07PM +0300, Josua Mayer wrote:
> The ADIN1300 supports generating certain clocks on its GP_CLK pin, as
> well as providing the reference clock on CLK25_REF.
> 
> Add DT properties to configure both pins.
> 
> Signed-off-by: Josua Mayer <josua@solid-run.com>

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

    Andrew

Re: [PATCH v2 3/3] ARM: dts: imx6qdl-sr-som: update phy configuration for som revision 1.9

[Andrew Lunn]

On Tue, Apr 19, 2022 at 01:27:09PM +0300, Josua Mayer wrote:
> Since SoM revision 1.9 the PHY has been replaced with an ADIN1300,
> add an entry for it next to the original.
> 
> Co-developed-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> ---
> V1 -> V2: changed dts property name
> 
>  arch/arm/boot/dts/imx6qdl-sr-som.dtsi | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
> index f86efd0ccc40..d46182095d79 100644
> --- a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
> @@ -83,6 +83,12 @@ ethernet-phy@4 {
>  			qca,clk-out-frequency = <125000000>;
>  			qca,smarteee-tw-us-1g = <24>;
>  		};
> +
> +		/* ADIN1300 (som rev 1.9 or later) */
> +		ethernet-phy@1 {
> +			reg = <1>;
> +			adi,phy-output-clock = "125mhz-free-running";
> +		};

There is currently the comment:

                 * The PHY can appear at either address 0 or 4 due to the
                 * configuration (LED) pin not being pulled sufficiently.
                 */

It would be good to add another comment about this PHY at address 1.

   Andrew

Re: [PATCH v2 3/3] ARM: dts: imx6qdl-sr-som: update phy configuration for som revision 1.9

[Russell King (Oracle)]

On Thu, Apr 21, 2022 at 02:27:16PM +0200, Andrew Lunn wrote:
> On Tue, Apr 19, 2022 at 01:27:09PM +0300, Josua Mayer wrote:
> > Since SoM revision 1.9 the PHY has been replaced with an ADIN1300,
> > add an entry for it next to the original.
> > 
> > Co-developed-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > Signed-off-by: Josua Mayer <josua@solid-run.com>
> > ---
> > V1 -> V2: changed dts property name
> > 
> >  arch/arm/boot/dts/imx6qdl-sr-som.dtsi | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
> > index f86efd0ccc40..d46182095d79 100644
> > --- a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
> > +++ b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
> > @@ -83,6 +83,12 @@ ethernet-phy@4 {
> >  			qca,clk-out-frequency = <125000000>;
> >  			qca,smarteee-tw-us-1g = <24>;
> >  		};
> > +
> > +		/* ADIN1300 (som rev 1.9 or later) */
> > +		ethernet-phy@1 {
> > +			reg = <1>;
> > +			adi,phy-output-clock = "125mhz-free-running";
> > +		};
> 
> There is currently the comment:
> 
>                  * The PHY can appear at either address 0 or 4 due to the
>                  * configuration (LED) pin not being pulled sufficiently.
>                  */
> 
> It would be good to add another comment about this PHY at address 1.

There is an issue with this approach. Listing the "possible" PHYs in DT
so we can configure them leads to the kernel complaining at boot time
with:

mdio_bus 2188000.ethernet-1: MDIO device at address 4 is missing.

So with this patch, we'll also get:

mdio_bus 2188000.ethernet-1: MDIO device at address 1 is missing.

which is not great for the user to see. Arguably though, it's down to
broken hardware design in the case of the AR8035, since this is caused
by insufficient pull on the LED pin to ensure the hardware address is
reliable configured.

However, adding this for a rev 1.9 uSOM where we know that the PHY is
different matter. I think we should be aiming for a new revision of
DT for the uSOM with the AR8035 nodes removed and the ADIN added,
rather than trying to stuff them all into a single DT and have the
kernel complain about not just one missing PHY, but now two.

The only other ways around this that I can see would be to have some
way to flag in DT that the PHYs are "optional" - if they're not found
while probing the hardware, then don't whinge about them. Or have
u-boot discover which address the PHY is located, and update the DT
blob passed to the kernel to disable the PHY addresses that aren't
present. Or edit the DT to update the node name and reg property. Or
something along those lines.

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

Re: [PATCH v2 3/3] ARM: dts: imx6qdl-sr-som: update phy configuration for som revision 1.9

[Andrew Lunn]

> The only other ways around this that I can see would be to have some
> way to flag in DT that the PHYs are "optional" - if they're not found
> while probing the hardware, then don't whinge about them. Or have
> u-boot discover which address the PHY is located, and update the DT
> blob passed to the kernel to disable the PHY addresses that aren't
> present. Or edit the DT to update the node name and reg property. Or
> something along those lines.

uboot sounds like the best option. I don't know if we currently
support the status property for PHYs. Maybe the .dtsi file should have
them all status = "disabled"; and uboot can flip the populated ones to
"okay". Or maybe the other way around to handle older bootloaders.

	Andrew

Re: [PATCH v2 3/3] ARM: dts: imx6qdl-sr-som: update phy configuration for som revision 1.9

[Russell King (Oracle)]

On Thu, Apr 21, 2022 at 03:30:38PM +0200, Andrew Lunn wrote:
> > The only other ways around this that I can see would be to have some
> > way to flag in DT that the PHYs are "optional" - if they're not found
> > while probing the hardware, then don't whinge about them. Or have
> > u-boot discover which address the PHY is located, and update the DT
> > blob passed to the kernel to disable the PHY addresses that aren't
> > present. Or edit the DT to update the node name and reg property. Or
> > something along those lines.
> 
> uboot sounds like the best option. I don't know if we currently
> support the status property for PHYs. Maybe the .dtsi file should have
> them all status = "disabled"; and uboot can flip the populated ones to
> "okay". Or maybe the other way around to handle older bootloaders.

... which would immediately regress the networking on all SolidRun iMX6
platforms when booting "new" DT with existing u-boot, so clearly that
isn't a possible solution.

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

Re: [PATCH v2 2/3] net: phy: adin: add support for clock output

[Josua Mayer]

\o/

I am going to fix this by using NULL in v3.
Is there any other feedback I should take into account on this patch?

- Josua Mayer

Am 21.04.22 um 09:45 schrieb kernel test robot:
> Hi Josua,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on robh/for-next]
> [also build test WARNING on net/master net-next/master v5.18-rc3 next-20220420]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Josua-Mayer/dt-bindings-net-adin-document-phy-clock-output-properties/20220419-192719
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
> config: openrisc-randconfig-s032-20220420 (https://download.01.org/0day-ci/archive/20220421/202204211324.qgcPMycQ-lkp@intel.com/config)
> compiler: or1k-linux-gcc (GCC) 11.2.0
> reproduce:
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # apt-get install sparse
>          # sparse version: v0.6.4-dirty
>          # https://github.com/intel-lab-lkp/linux/commit/74d856f1c89a6534fd58889f20ad4b481b8191c9
>          git remote add linux-review https://github.com/intel-lab-lkp/linux
>          git fetch --no-tags linux-review Josua-Mayer/dt-bindings-net-adin-document-phy-clock-output-properties/20220419-192719
>          git checkout 74d856f1c89a6534fd58889f20ad4b481b8191c9
>          # save the config file
>          mkdir build_dir && cp config build_dir/.config
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=openrisc SHELL=/bin/bash drivers/net/phy/
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
>
> sparse warnings: (new ones prefixed by >>)
>>> drivers/net/phy/adin.c:448:27: sparse: sparse: Using plain integer as NULL pointer
> vim +448 drivers/net/phy/adin.c
>
>     444	
>     445	static int adin_config_clk_out(struct phy_device *phydev)
>     446	{
>     447		struct device *dev = &phydev->mdio.dev;
>   > 448		const char *val = 0;
>     449		u8 sel = 0;
>     450	
>     451		device_property_read_string(dev, "adi,phy-output-clock", &val);
>     452		if(!val) {
>     453			/* property not present, do not enable GP_CLK pin */
>     454		} else if(strcmp(val, "25mhz-reference") == 0) {
>     455			sel |= ADIN1300_GE_CLK_CFG_25;
>     456		} else if(strcmp(val, "125mhz-free-running") == 0) {
>     457			sel |= ADIN1300_GE_CLK_CFG_FREE_125;
>     458		} else if(strcmp(val, "125mhz-recovered") == 0) {
>     459			sel |= ADIN1300_GE_CLK_CFG_RCVR_125;
>     460		} else if(strcmp(val, "adaptive-free-running") == 0) {
>     461			sel |= ADIN1300_GE_CLK_CFG_HRT_FREE;
>     462		} else if(strcmp(val, "adaptive-recovered") == 0) {
>     463			sel |= ADIN1300_GE_CLK_CFG_HRT_RCVR;
>     464		} else {
>     465			phydev_err(phydev, "invalid adi,phy-output-clock\n");
>     466			return -EINVAL;
>     467		}
>     468	
>     469		if(device_property_read_bool(dev, "adi,phy-output-reference-clock"))
>     470			sel |= ADIN1300_GE_CLK_CFG_REF_EN;
>     471	
>     472		return phy_modify_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_CLK_CFG_REG,
>     473				      ADIN1300_GE_CLK_CFG_MASK, sel);
>     474	}
>     475	
>

Re: [PATCH v2 2/3] net: phy: adin: add support for clock output

[Josua Mayer]

[-- Attachment #1: Type: text/plain, Size: 3584 bytes --]

\o/

I am going to fix this by using NULL in v3.
Is there any other feedback I should take into account on this patch?

- Josua Mayer

Am 21.04.22 um 09:45 schrieb kernel test robot:
> Hi Josua,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on robh/for-next]
> [also build test WARNING on net/master net-next/master v5.18-rc3 next-20220420]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Josua-Mayer/dt-bindings-net-adin-document-phy-clock-output-properties/20220419-192719
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
> config: openrisc-randconfig-s032-20220420 (https://download.01.org/0day-ci/archive/20220421/202204211324.qgcPMycQ-lkp(a)intel.com/config)
> compiler: or1k-linux-gcc (GCC) 11.2.0
> reproduce:
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # apt-get install sparse
>          # sparse version: v0.6.4-dirty
>          # https://github.com/intel-lab-lkp/linux/commit/74d856f1c89a6534fd58889f20ad4b481b8191c9
>          git remote add linux-review https://github.com/intel-lab-lkp/linux
>          git fetch --no-tags linux-review Josua-Mayer/dt-bindings-net-adin-document-phy-clock-output-properties/20220419-192719
>          git checkout 74d856f1c89a6534fd58889f20ad4b481b8191c9
>          # save the config file
>          mkdir build_dir && cp config build_dir/.config
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=openrisc SHELL=/bin/bash drivers/net/phy/
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
>
> sparse warnings: (new ones prefixed by >>)
>>> drivers/net/phy/adin.c:448:27: sparse: sparse: Using plain integer as NULL pointer
> vim +448 drivers/net/phy/adin.c
>
>     444	
>     445	static int adin_config_clk_out(struct phy_device *phydev)
>     446	{
>     447		struct device *dev = &phydev->mdio.dev;
>   > 448		const char *val = 0;
>     449		u8 sel = 0;
>     450	
>     451		device_property_read_string(dev, "adi,phy-output-clock", &val);
>     452		if(!val) {
>     453			/* property not present, do not enable GP_CLK pin */
>     454		} else if(strcmp(val, "25mhz-reference") == 0) {
>     455			sel |= ADIN1300_GE_CLK_CFG_25;
>     456		} else if(strcmp(val, "125mhz-free-running") == 0) {
>     457			sel |= ADIN1300_GE_CLK_CFG_FREE_125;
>     458		} else if(strcmp(val, "125mhz-recovered") == 0) {
>     459			sel |= ADIN1300_GE_CLK_CFG_RCVR_125;
>     460		} else if(strcmp(val, "adaptive-free-running") == 0) {
>     461			sel |= ADIN1300_GE_CLK_CFG_HRT_FREE;
>     462		} else if(strcmp(val, "adaptive-recovered") == 0) {
>     463			sel |= ADIN1300_GE_CLK_CFG_HRT_RCVR;
>     464		} else {
>     465			phydev_err(phydev, "invalid adi,phy-output-clock\n");
>     466			return -EINVAL;
>     467		}
>     468	
>     469		if(device_property_read_bool(dev, "adi,phy-output-reference-clock"))
>     470			sel |= ADIN1300_GE_CLK_CFG_REF_EN;
>     471	
>     472		return phy_modify_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_CLK_CFG_REG,
>     473				      ADIN1300_GE_CLK_CFG_MASK, sel);
>     474	}
>     475	
>

Re: [PATCH v2 3/3] ARM: dts: imx6qdl-sr-som: update phy configuration for som revision 1.9

[Josua Mayer]

Hi Russell,

Am 21.04.22 um 17:20 schrieb Russell King (Oracle):
> On Thu, Apr 21, 2022 at 03:30:38PM +0200, Andrew Lunn wrote:
>>> The only other ways around this that I can see would be to have some
>>> way to flag in DT that the PHYs are "optional" - if they're not found
>>> while probing the hardware, then don't whinge about them. Or have
>>> u-boot discover which address the PHY is located, and update the DT
>>> blob passed to the kernel to disable the PHY addresses that aren't
>>> present. Or edit the DT to update the node name and reg property. Or
>>> something along those lines.
>> uboot sounds like the best option. I don't know if we currently
>> support the status property for PHYs. Maybe the .dtsi file should have
>> them all status = "disabled"; and uboot can flip the populated ones to
>> "okay". Or maybe the other way around to handle older bootloaders.
> ... which would immediately regress the networking on all SolidRun iMX6
> platforms when booting "new" DT with existing u-boot, so clearly that
> isn't a possible solution.

So to summarize - you don't want to see a third phy spamming the console 
with probe errors ...

I think a combination of the suggestions would be doable:
- Add the new phy to dt, with status disabled
- keep the existing phys unchanged
- after probing in u-boot, disable the two old entries, and enable the 
new one

It is not very convenient since that means changes to u-boot are necessary,
but it can be done - and won't break existing users only updating Linux.

sincerely
Josua Mayer

[PATCH v3 0/3] adin: add support for clock output

[Josua Mayer]

This patch series adds support for configuring the two clock outputs of adin
1200 and 1300 PHYs. Certain network controllers require an external reference
clock which can be provided by the PHY.

One of the replies to v1 was asking why the common clock framework isn't used.
Currently no PHY driver has implemented providing a clock to the network
controller. Instead they rely on vendor extensions to make the appropriate
configuration. For example ar8035 uses qca,clk-out-frequency - this patchset
aimed to replicate the same functionality.

Finally the 125MHz free-running clock is enabled in the device-tree for
SolidRun i.MX6 SoMs, to support revisions 1.9 and later, where the original phy
has been replaced with an adin 1300.
To avoid introducing new warning messages during boot for SoMs before rev 1.9,
the status field of the new phy node is disabled by default, and will be
enabled by U-Boot on demand.

Changes since v2:
- set new phy node status to disabled
- fix integer-as-null-pointer compiler warning
  Reported-by: kernel test robot <lkp@intel.com>

Changes since v1:
- renamed device-tree property and changed to enum
- added device-tree property for second clock output
- implemented all bits from the clock configuration register

Josua Mayer (3):
  dt-bindings: net: adin: document phy clock output properties
  net: phy: adin: add support for clock output
  ARM: dts: imx6qdl-sr-som: update phy configuration for som revision
    1.9

 .../devicetree/bindings/net/adi,adin.yaml     | 17 +++++++
 arch/arm/boot/dts/imx6qdl-sr-som.dtsi         | 10 +++++
 drivers/net/phy/adin.c                        | 44 +++++++++++++++++++
 3 files changed, 71 insertions(+)

-- 
2.34.1

[PATCH v3 1/3] dt-bindings: net: adin: document phy clock output properties

[Josua Mayer]

The ADIN1300 supports generating certain clocks on its GP_CLK pin, as
well as providing the reference clock on CLK25_REF.

Add DT properties to configure both pins.

Signed-off-by: Josua Mayer <josua@solid-run.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
V1 -> V2: changed clkout property to enum
V1 -> V2: added property for CLK25_REF pin

 .../devicetree/bindings/net/adi,adin.yaml       | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
index 1129f2b58e98..3e0c6304f190 100644
--- a/Documentation/devicetree/bindings/net/adi,adin.yaml
+++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
@@ -36,6 +36,23 @@ properties:
     enum: [ 4, 8, 12, 16, 20, 24 ]
     default: 8
 
+  adi,phy-output-clock:
+    description: Select clock output on GP_CLK pin. Three clocks are available:
+      A 25MHz reference, a free-running 125MHz and a recovered 125MHz.
+      The phy can also automatically switch between the reference and the
+      respective 125MHz clocks based on its internal state.
+    $ref: /schemas/types.yaml#/definitions/string
+    enum:
+    - 25mhz-reference
+    - 125mhz-free-running
+    - 125mhz-recovered
+    - adaptive-free-running
+    - adaptive-recovered
+
+  adi,phy-output-reference-clock:
+    description: Enable 25MHz reference clock output on CLK25_REF pin.
+    $ref: /schemas/types.yaml#/definitions/flag
+
 unevaluatedProperties: false
 
 examples:
-- 
2.34.1

[PATCH v3 2/3] net: phy: adin: add support for clock output

[Josua Mayer]

The ADIN1300 supports generating certain clocks on its GP_CLK pin, as
well as providing the reference clock on CLK25_REF.

Add support for selecting the clock via device-tree properties.

Co-developed-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Josua Mayer<josua@solid-run.com>
---
V2 -> V3: fix integer-as-null-pointer compiler warning
V1 -> V2: revised dts property name for clock(s)
V1 -> V2: implemented all 6 bits in the clock configuration register

 drivers/net/phy/adin.c | 44 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 5ce6da62cc8e..2de3eaddfb8e 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -99,6 +99,15 @@
 #define ADIN1300_GE_SOFT_RESET_REG		0xff0c
 #define   ADIN1300_GE_SOFT_RESET		BIT(0)
 
+#define ADIN1300_GE_CLK_CFG_REG			0xff1f
+#define   ADIN1300_GE_CLK_CFG_MASK		GENMASK(5, 0)
+#define   ADIN1300_GE_CLK_CFG_RCVR_125		BIT(5)
+#define   ADIN1300_GE_CLK_CFG_FREE_125		BIT(4)
+#define   ADIN1300_GE_CLK_CFG_REF_EN		BIT(3)
+#define   ADIN1300_GE_CLK_CFG_HRT_RCVR		BIT(2)
+#define   ADIN1300_GE_CLK_CFG_HRT_FREE		BIT(1)
+#define   ADIN1300_GE_CLK_CFG_25		BIT(0)
+
 #define ADIN1300_GE_RGMII_CFG_REG		0xff23
 #define   ADIN1300_GE_RGMII_RX_MSK		GENMASK(8, 6)
 #define   ADIN1300_GE_RGMII_RX_SEL(x)		\
@@ -433,6 +442,37 @@ static int adin_set_tunable(struct phy_device *phydev,
 	}
 }
 
+static int adin_config_clk_out(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	const char *val = NULL;
+	u8 sel = 0;
+
+	device_property_read_string(dev, "adi,phy-output-clock", &val);
+	if(!val) {
+		/* property not present, do not enable GP_CLK pin */
+	} else if(strcmp(val, "25mhz-reference") == 0) {
+		sel |= ADIN1300_GE_CLK_CFG_25;
+	} else if(strcmp(val, "125mhz-free-running") == 0) {
+		sel |= ADIN1300_GE_CLK_CFG_FREE_125;
+	} else if(strcmp(val, "125mhz-recovered") == 0) {
+		sel |= ADIN1300_GE_CLK_CFG_RCVR_125;
+	} else if(strcmp(val, "adaptive-free-running") == 0) {
+		sel |= ADIN1300_GE_CLK_CFG_HRT_FREE;
+	} else if(strcmp(val, "adaptive-recovered") == 0) {
+		sel |= ADIN1300_GE_CLK_CFG_HRT_RCVR;
+	} else {
+		phydev_err(phydev, "invalid adi,phy-output-clock\n");
+		return -EINVAL;
+	}
+
+	if(device_property_read_bool(dev, "adi,phy-output-reference-clock"))
+		sel |= ADIN1300_GE_CLK_CFG_REF_EN;
+
+	return phy_modify_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_CLK_CFG_REG,
+			      ADIN1300_GE_CLK_CFG_MASK, sel);
+}
+
 static int adin_config_init(struct phy_device *phydev)
 {
 	int rc;
@@ -455,6 +495,10 @@ static int adin_config_init(struct phy_device *phydev)
 	if (rc < 0)
 		return rc;
 
+	rc = adin_config_clk_out(phydev);
+	if (rc < 0)
+		return rc;
+
 	phydev_dbg(phydev, "PHY is using mode '%s'\n",
 		   phy_modes(phydev->interface));
 
-- 
2.34.1

[PATCH v3 3/3] ARM: dts: imx6qdl-sr-som: update phy configuration for som revision 1.9

[Josua Mayer]

Since SoM revision 1.9 the PHY has been replaced with an ADIN1300,
add an entry for it next to the original.

As Russell King pointed out, additional phy nodes cause warnings like:
mdio_bus 2188000.ethernet-1: MDIO device at address 1 is missing
To avoid this the new node has its status set to disabled. U-Boot will
be modified to enable the appropriate phy node after probing.

The existing ar8035 nodes have to stay enabled by default to avoid
breaking existing systems when they update Linux only.

Co-developed-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
V2 -> V3: new phy node status set disabled
V1 -> V2: changed dts property name

 arch/arm/boot/dts/imx6qdl-sr-som.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
index f86efd0ccc40..ce543e325cd3 100644
--- a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
@@ -83,6 +83,16 @@ ethernet-phy@4 {
 			qca,clk-out-frequency = <125000000>;
 			qca,smarteee-tw-us-1g = <24>;
 		};
+
+		/*
+		 * ADIN1300 (som rev 1.9 or later) is always at address 1. It
+		 * will be enabled automatically by U-Boot if detected.
+		 */
+		ethernet-phy@1 {
+			reg = <1>;
+			adi,phy-output-clock = "125mhz-free-running";
+			status = "disabled";
+		};
 	};
 };
 
-- 
2.34.1

Re: [PATCH v3 2/3] net: phy: adin: add support for clock output

[Andrew Lunn]

> +static int adin_config_clk_out(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	const char *val = NULL;
> +	u8 sel = 0;
> +
> +	device_property_read_string(dev, "adi,phy-output-clock", &val);
> +	if(!val) {

I'm pretty sure the coding style requires a space between if and (.

Did you use checkpatch on this?

    Andrew

Re: [PATCH v3 2/3] net: phy: adin: add support for clock output

[Josua Mayer]

\o/

Am 28.04.22 um 15:21 schrieb Andrew Lunn:
>> +static int adin_config_clk_out(struct phy_device *phydev)
>> +{
>> +	struct device *dev = &phydev->mdio.dev;
>> +	const char *val = NULL;
>> +	u8 sel = 0;
>> +
>> +	device_property_read_string(dev, "adi,phy-output-clock", &val);
>> +	if(!val) {
> I'm pretty sure the coding style requires a space between if and (.
In fact it does :(
>
> Did you use checkpatch on this?
I remember doing it, but my mind is playing tricks on me - as right now
checkpatch is clearly telling me 7 occurences of this style violation ...

Thank you for the fast reply, I'll make sure to fix this in a v4, if any.
Do you want a v4 for this? Or is it worth waiting for more feedback now?

sincerely
Josua Mayer

Re: [PATCH v3 2/3] net: phy: adin: add support for clock output

[Andrew Lunn]

> Thank you for the fast reply, I'll make sure to fix this in a v4, if any.
> Do you want a v4 for this? Or is it worth waiting for more feedback now?

As a general rule of thumb, wait at least one day before posting a new
version.

   Andrew

Re: [PATCH v3 3/3] ARM: dts: imx6qdl-sr-som: update phy configuration for som revision 1.9

[Shawn Guo]

On Thu, Apr 28, 2022 at 11:28:48AM +0300, Josua Mayer wrote:
> Since SoM revision 1.9 the PHY has been replaced with an ADIN1300,
> add an entry for it next to the original.
> 
> As Russell King pointed out, additional phy nodes cause warnings like:
> mdio_bus 2188000.ethernet-1: MDIO device at address 1 is missing
> To avoid this the new node has its status set to disabled. U-Boot will
> be modified to enable the appropriate phy node after probing.
> 
> The existing ar8035 nodes have to stay enabled by default to avoid
> breaking existing systems when they update Linux only.
> 
> Co-developed-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> ---
> V2 -> V3: new phy node status set disabled
> V1 -> V2: changed dts property name
> 
>  arch/arm/boot/dts/imx6qdl-sr-som.dtsi | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
> index f86efd0ccc40..ce543e325cd3 100644
> --- a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
> @@ -83,6 +83,16 @@ ethernet-phy@4 {
>  			qca,clk-out-frequency = <125000000>;
>  			qca,smarteee-tw-us-1g = <24>;
>  		};
> +
> +		/*
> +		 * ADIN1300 (som rev 1.9 or later) is always at address 1. It
> +		 * will be enabled automatically by U-Boot if detected.
> +		 */
> +		ethernet-phy@1 {
> +			reg = <1>;
> +			adi,phy-output-clock = "125mhz-free-running";

Hmm, I failed to find binding doc for this.

Shawn

> +			status = "disabled";
> +		};
>  	};
>  };
>  
> -- 
> 2.34.1
> 

Re: [PATCH v3 1/3] dt-bindings: net: adin: document phy clock output properties

[Josua Mayer]

Hi Shawn,

Thank you for looking at patch 3 in this series, in case you missed it - 
the adi,phy-output-clock property does not yet exist in bindings, I am 
trying to get that one added.

Am 28.04.22 um 11:28 schrieb Josua Mayer:
> The ADIN1300 supports generating certain clocks on its GP_CLK pin, as
> well as providing the reference clock on CLK25_REF.
>
> Add DT properties to configure both pins.
>
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
> V1 -> V2: changed clkout property to enum
> V1 -> V2: added property for CLK25_REF pin
>
>   .../devicetree/bindings/net/adi,adin.yaml       | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
> index 1129f2b58e98..3e0c6304f190 100644
> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml
> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
> @@ -36,6 +36,23 @@ properties:
>       enum: [ 4, 8, 12, 16, 20, 24 ]
>       default: 8
>   
> +  adi,phy-output-clock:
> +    description: Select clock output on GP_CLK pin. Three clocks are available:
> +      A 25MHz reference, a free-running 125MHz and a recovered 125MHz.
> +      The phy can also automatically switch between the reference and the
> +      respective 125MHz clocks based on its internal state.
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum:
> +    - 25mhz-reference
> +    - 125mhz-free-running
> +    - 125mhz-recovered
> +    - adaptive-free-running
> +    - adaptive-recovered
> +
> +  adi,phy-output-reference-clock:
> +    description: Enable 25MHz reference clock output on CLK25_REF pin.
> +    $ref: /schemas/types.yaml#/definitions/flag
> +
>   unevaluatedProperties: false
>   
>   examples:
- Josua Mayer

Re: [PATCH v3 1/3] dt-bindings: net: adin: document phy clock output properties

[Krzysztof Kozlowski]

On 28/04/2022 10:28, Josua Mayer wrote:

Thank you for your patch. There is something to discuss/improve.

> 
> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
> index 1129f2b58e98..3e0c6304f190 100644
> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml
> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
> @@ -36,6 +36,23 @@ properties:
>      enum: [ 4, 8, 12, 16, 20, 24 ]
>      default: 8
>  
> +  adi,phy-output-clock:
> +    description: Select clock output on GP_CLK pin. Three clocks are available:
> +      A 25MHz reference, a free-running 125MHz and a recovered 125MHz.
> +      The phy can also automatically switch between the reference and the
> +      respective 125MHz clocks based on its internal state.
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum:
> +    - 25mhz-reference
> +    - 125mhz-free-running
> +    - 125mhz-recovered
> +    - adaptive-free-running
> +    - adaptive-recovered

Missing two spaces of indentation for all these items.

> +
> +  adi,phy-output-reference-clock:
> +    description: Enable 25MHz reference clock output on CLK25_REF pin.
> +    $ref: /schemas/types.yaml#/definitions/flag

This could be just "type:boolean".

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: net: adin: document phy clock output properties

[Josua Mayer]

\o/

Am 05.05.22 um 23:24 schrieb Krzysztof Kozlowski:
> On 28/04/2022 10:28, Josua Mayer wrote:
>
> Thank you for your patch. There is something to discuss/improve.
Thank you for taking a look.
>> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
>> index 1129f2b58e98..3e0c6304f190 100644
>> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml
>> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
>> @@ -36,6 +36,23 @@ properties:
>>       enum: [ 4, 8, 12, 16, 20, 24 ]
>>       default: 8
>>   
>> +  adi,phy-output-clock:
>> +    description: Select clock output on GP_CLK pin. Three clocks are available:
>> +      A 25MHz reference, a free-running 125MHz and a recovered 125MHz.
>> +      The phy can also automatically switch between the reference and the
>> +      respective 125MHz clocks based on its internal state.
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    enum:
>> +    - 25mhz-reference
>> +    - 125mhz-free-running
>> +    - 125mhz-recovered
>> +    - adaptive-free-running
>> +    - adaptive-recovered
> Missing two spaces of indentation for all these items.
Will add in v4, thank you.
>
>> +
>> +  adi,phy-output-reference-clock:
>> +    description: Enable 25MHz reference clock output on CLK25_REF pin.
>> +    $ref: /schemas/types.yaml#/definitions/flag
> This could be just "type:boolean".
Yes, it could be boolean, and default to false.
So ... I figured its a flag, but whether to make it a flag or boolean is 
better I do not know.
> Best regards,
> Krzysztof
sincerely
- Josua Mayer

Re: [PATCH v3 1/3] dt-bindings: net: adin: document phy clock output properties

[Krzysztof Kozlowski]

On 08/05/2022 11:57, Josua Mayer wrote:
>>> +  adi,phy-output-reference-clock:
>>> +    description: Enable 25MHz reference clock output on CLK25_REF pin.
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>> This could be just "type:boolean".
> Yes, it could be boolean, and default to false.
> So ... I figured its a flag, but whether to make it a flag or boolean is 
> better I do not know.
As I said, use "type:boolean", less typing and it is equivalent.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: net: adin: document phy clock output properties

[Josua Mayer]

Am 05.05.22 um 23:24 schrieb Krzysztof Kozlowski:
> On 28/04/2022 10:28, Josua Mayer wrote:
>
> Thank you for your patch. There is something to discuss/improve.
>
>> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
>> index 1129f2b58e98..3e0c6304f190 100644
>> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml
>> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
>> @@ -36,6 +36,23 @@ properties:
>>       enum: [ 4, 8, 12, 16, 20, 24 ]
>>       default: 8
>>   
>> +  adi,phy-output-clock:
>> +    description: Select clock output on GP_CLK pin. Three clocks are available:
>> +      A 25MHz reference, a free-running 125MHz and a recovered 125MHz.
>> +      The phy can also automatically switch between the reference and the
>> +      respective 125MHz clocks based on its internal state.
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    enum:
>> +    - 25mhz-reference
>> +    - 125mhz-free-running
>> +    - 125mhz-recovered
>> +    - adaptive-free-running
>> +    - adaptive-recovered
> Missing two spaces of indentation for all these items.
>
>> +
>> +  adi,phy-output-reference-clock:
>> +    description: Enable 25MHz reference clock output on CLK25_REF pin.
>> +    $ref: /schemas/types.yaml#/definitions/flag
> This could be just "type:boolean".
I will change it accordingly, thanks.
> Best regards,
> Krzysztof

[PATCH v4 0/3] adin: add support for clock output

[Josua Mayer]

This patch series adds support for configuring the two clock outputs of adin
1200 and 1300 PHYs. Certain network controllers require an external reference
clock which can be provided by the PHY.

One of the replies to v1 was asking why the common clock framework isn't used.
Currently no PHY driver has implemented providing a clock to the network
controller. Instead they rely on vendor extensions to make the appropriate
configuration. For example ar8035 uses qca,clk-out-frequency - this patchset
aimed to replicate the same functionality.

Finally the 125MHz free-running clock is enabled in the device-tree for
SolidRun i.MX6 SoMs, to support revisions 1.9 and later, where the original phy
has been replaced with an adin 1300.
To avoid introducing new warning messages during boot for SoMs before rev 1.9,
the status field of the new phy node is disabled by default, and will be
enabled by U-Boot on demand.

Changes since v3:
- fix coding style violations reported by Andrew and checkpatch
- changed type of adi,phy-output-reference-clock from flag to boolean

Changes since v2:
- set new phy node status to disabled
- fix integer-as-null-pointer compiler warning
  Reported-by: kernel test robot <lkp@intel.com>

Changes since v1:
- renamed device-tree property and changed to enum
- added device-tree property for second clock output
- implemented all bits from the clock configuration register

Josua Mayer (3):
  dt-bindings: net: adin: document phy clock output properties
  net: phy: adin: add support for clock output
  ARM: dts: imx6qdl-sr-som: update phy configuration for som revision
    1.9

 .../devicetree/bindings/net/adi,adin.yaml     | 17 +++++++
 arch/arm/boot/dts/imx6qdl-sr-som.dtsi         | 10 +++++
 drivers/net/phy/adin.c                        | 44 +++++++++++++++++++
 3 files changed, 71 insertions(+)

-- 
2.35.3

[PATCH v4 1/3] dt-bindings: net: adin: document phy clock output properties

[Josua Mayer]

The ADIN1300 supports generating certain clocks on its GP_CLK pin, as
well as providing the reference clock on CLK25_REF.

Add DT properties to configure both pins.

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
V3 -> V4: changed type of adi,phy-output-reference-clock to boolean
V1 -> V2: changed clkout property to enum
V1 -> V2: added property for CLK25_REF pin

 .../devicetree/bindings/net/adi,adin.yaml       | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
index 1129f2b58e98..cc4f128222ba 100644
--- a/Documentation/devicetree/bindings/net/adi,adin.yaml
+++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
@@ -36,6 +36,23 @@ properties:
     enum: [ 4, 8, 12, 16, 20, 24 ]
     default: 8
 
+  adi,phy-output-clock:
+    description: Select clock output on GP_CLK pin. Three clocks are available:
+      A 25MHz reference, a free-running 125MHz and a recovered 125MHz.
+      The phy can also automatically switch between the reference and the
+      respective 125MHz clocks based on its internal state.
+    $ref: /schemas/types.yaml#/definitions/string
+    enum:
+      - 25mhz-reference
+      - 125mhz-free-running
+      - 125mhz-recovered
+      - adaptive-free-running
+      - adaptive-recovered
+
+  adi,phy-output-reference-clock:
+    description: Enable 25MHz reference clock output on CLK25_REF pin.
+    type: boolean
+
 unevaluatedProperties: false
 
 examples:
-- 
2.35.3

[PATCH v4 2/3] net: phy: adin: add support for clock output

[Josua Mayer]

The ADIN1300 supports generating certain clocks on its GP_CLK pin, as
well as providing the reference clock on CLK25_REF.

Add support for selecting the clock via device-tree properties.

Co-developed-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Josua Mayer<josua@solid-run.com>
---
V3 -> V4: fix coding style violations reported by Andrew and checkpatch
V2 -> V3: fix integer-as-null-pointer compiler warning
V1 -> V2: revised dts property name for clock(s)
V1 -> V2: implemented all 6 bits in the clock configuration register

 drivers/net/phy/adin.c | 44 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 5ce6da62cc8e..1341249d8d2c 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -99,6 +99,15 @@
 #define ADIN1300_GE_SOFT_RESET_REG		0xff0c
 #define   ADIN1300_GE_SOFT_RESET		BIT(0)
 
+#define ADIN1300_GE_CLK_CFG_REG			0xff1f
+#define   ADIN1300_GE_CLK_CFG_MASK		GENMASK(5, 0)
+#define   ADIN1300_GE_CLK_CFG_RCVR_125		BIT(5)
+#define   ADIN1300_GE_CLK_CFG_FREE_125		BIT(4)
+#define   ADIN1300_GE_CLK_CFG_REF_EN		BIT(3)
+#define   ADIN1300_GE_CLK_CFG_HRT_RCVR		BIT(2)
+#define   ADIN1300_GE_CLK_CFG_HRT_FREE		BIT(1)
+#define   ADIN1300_GE_CLK_CFG_25		BIT(0)
+
 #define ADIN1300_GE_RGMII_CFG_REG		0xff23
 #define   ADIN1300_GE_RGMII_RX_MSK		GENMASK(8, 6)
 #define   ADIN1300_GE_RGMII_RX_SEL(x)		\
@@ -433,6 +442,37 @@ static int adin_set_tunable(struct phy_device *phydev,
 	}
 }
 
+static int adin_config_clk_out(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	const char *val = NULL;
+	u8 sel = 0;
+
+	device_property_read_string(dev, "adi,phy-output-clock", &val);
+	if (!val) {
+		/* property not present, do not enable GP_CLK pin */
+	} else if (strcmp(val, "25mhz-reference") == 0) {
+		sel |= ADIN1300_GE_CLK_CFG_25;
+	} else if (strcmp(val, "125mhz-free-running") == 0) {
+		sel |= ADIN1300_GE_CLK_CFG_FREE_125;
+	} else if (strcmp(val, "125mhz-recovered") == 0) {
+		sel |= ADIN1300_GE_CLK_CFG_RCVR_125;
+	} else if (strcmp(val, "adaptive-free-running") == 0) {
+		sel |= ADIN1300_GE_CLK_CFG_HRT_FREE;
+	} else if (strcmp(val, "adaptive-recovered") == 0) {
+		sel |= ADIN1300_GE_CLK_CFG_HRT_RCVR;
+	} else {
+		phydev_err(phydev, "invalid adi,phy-output-clock\n");
+		return -EINVAL;
+	}
+
+	if (device_property_read_bool(dev, "adi,phy-output-reference-clock"))
+		sel |= ADIN1300_GE_CLK_CFG_REF_EN;
+
+	return phy_modify_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_CLK_CFG_REG,
+			      ADIN1300_GE_CLK_CFG_MASK, sel);
+}
+
 static int adin_config_init(struct phy_device *phydev)
 {
 	int rc;
@@ -455,6 +495,10 @@ static int adin_config_init(struct phy_device *phydev)
 	if (rc < 0)
 		return rc;
 
+	rc = adin_config_clk_out(phydev);
+	if (rc < 0)
+		return rc;
+
 	phydev_dbg(phydev, "PHY is using mode '%s'\n",
 		   phy_modes(phydev->interface));
 
-- 
2.35.3

[PATCH v4 3/3] ARM: dts: imx6qdl-sr-som: update phy configuration for som revision 1.9

[Josua Mayer]

Since SoM revision 1.9 the PHY has been replaced with an ADIN1300,
add an entry for it next to the original.

As Russell King pointed out, additional phy nodes cause warnings like:
mdio_bus 2188000.ethernet-1: MDIO device at address 1 is missing
To avoid this the new node has its status set to disabled. U-Boot will
be modified to enable the appropriate phy node after probing.

The existing ar8035 nodes have to stay enabled by default to avoid
breaking existing systems when they update Linux only.

Co-developed-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
V2 -> V3: new phy node status set disabled
V1 -> V2: changed dts property name

 arch/arm/boot/dts/imx6qdl-sr-som.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
index f86efd0ccc40..ce543e325cd3 100644
--- a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
@@ -83,6 +83,16 @@ ethernet-phy@4 {
 			qca,clk-out-frequency = <125000000>;
 			qca,smarteee-tw-us-1g = <24>;
 		};
+
+		/*
+		 * ADIN1300 (som rev 1.9 or later) is always at address 1. It
+		 * will be enabled automatically by U-Boot if detected.
+		 */
+		ethernet-phy@1 {
+			reg = <1>;
+			adi,phy-output-clock = "125mhz-free-running";
+			status = "disabled";
+		};
 	};
 };
 
-- 
2.35.3

Re: [PATCH v2 3/3] ARM: dts: imx6qdl-sr-som: update phy configuration for som revision 1.9

[Russell King (Oracle)]

On Wed, Apr 27, 2022 at 10:15:31AM +0300, Josua Mayer wrote:
> Hi Russell,
> 
> Am 21.04.22 um 17:20 schrieb Russell King (Oracle):
> > On Thu, Apr 21, 2022 at 03:30:38PM +0200, Andrew Lunn wrote:
> > > > The only other ways around this that I can see would be to have some
> > > > way to flag in DT that the PHYs are "optional" - if they're not found
> > > > while probing the hardware, then don't whinge about them. Or have
> > > > u-boot discover which address the PHY is located, and update the DT
> > > > blob passed to the kernel to disable the PHY addresses that aren't
> > > > present. Or edit the DT to update the node name and reg property. Or
> > > > something along those lines.
> > > uboot sounds like the best option. I don't know if we currently
> > > support the status property for PHYs. Maybe the .dtsi file should have
> > > them all status = "disabled"; and uboot can flip the populated ones to
> > > "okay". Or maybe the other way around to handle older bootloaders.
> > ... which would immediately regress the networking on all SolidRun iMX6
> > platforms when booting "new" DT with existing u-boot, so clearly that
> > isn't a possible solution.
> 
> So to summarize - you don't want to see a third phy spamming the console
> with probe errors ...

Exactly - it's bad enough that we have to list two PHYs because the PHY
could appear at either address 0 or address 4 depending on the direction
of the wind on any given day - and all because the PHY uses the LED pin
to determine its address, and which doesn't give a strong enough pull
that the address can be reliably determined. Every time the PHY gets a
hardware reset, it can change its address.

> I think a combination of the suggestions would be doable:
> - Add the new phy to dt, with status disabled
> - keep the existing phys unchanged
> - after probing in u-boot, disable the two old entries, and enable the new
> one

Exactly.

> It is not very convenient since that means changes to u-boot are necessary,
> but it can be done - and won't break existing users only updating Linux.

We wouldn't have the first problem of needing two PHYs if the hardware
had been fixed after I reported the problem...

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

Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock output properties

[Krzysztof Kozlowski]

On 09/05/2022 16:36, Josua Mayer wrote:
> The ADIN1300 supports generating certain clocks on its GP_CLK pin, as
> well as providing the reference clock on CLK25_REF.
> 
> Add DT properties to configure both pins.
> 
> Signed-off-by: Josua Mayer <josua@solid-run.com>

Don't attach new patchsets to some old patchsets. Basically this makes
the thread buried deep and possible ignored.

> ---
> V3 -> V4: changed type of adi,phy-output-reference-clock to boolean
> V1 -> V2: changed clkout property to enum
> V1 -> V2: added property for CLK25_REF pin
> 
>  .../devicetree/bindings/net/adi,adin.yaml       | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 


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


Best regards,
Krzysztof

Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock output properties

[Jakub Kicinski]

On Mon,  9 May 2022 17:36:33 +0300 Josua Mayer wrote:
> +  adi,phy-output-clock:
> +    description: Select clock output on GP_CLK pin. Three clocks are available:
> +      A 25MHz reference, a free-running 125MHz and a recovered 125MHz.
> +      The phy can also automatically switch between the reference and the
> +      respective 125MHz clocks based on its internal state.
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum:
> +      - 25mhz-reference
> +      - 125mhz-free-running
> +      - 125mhz-recovered
> +      - adaptive-free-running
> +      - adaptive-recovered

I'm still not convinced that exposing the free running vs recovered
distinction from the start is a good idea. Intuitively it'd seem that
it's better to use the recovered clock to feed the wire side of the MAC,
this patch set uses the free running. So I'd personally strip the last 
part off and add it later if needed.

But I won't fuss, if we get an ack from one of the PHY maintainers -
I'll merge as is.

Andrew? 

Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock

[Michael Walle]

> On Mon,  9 May 2022 17:36:33 +0300 Josua Mayer wrote:
> > +  adi,phy-output-clock:
> > +    description: Select clock output on GP_CLK pin. Three clocks are available:
> > +      A 25MHz reference, a free-running 125MHz and a recovered 125MHz.
> > +      The phy can also automatically switch between the reference and the
> > +      respective 125MHz clocks based on its internal state.
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum:
> > +      - 25mhz-reference
> > +      - 125mhz-free-running
> > +      - 125mhz-recovered
> > +      - adaptive-free-running
> > +      - adaptive-recovered
> 
> I'm still not convinced that exposing the free running vs recovered
> distinction from the start is a good idea. Intuitively it'd seem that
> it's better to use the recovered clock to feed the wire side of the MAC,
> this patch set uses the free running. So I'd personally strip the last 
> part off and add it later if needed.

FWIW, the recovered clock only works if there is a link. AFAIR on the
AR8031 you can have the free-running one enabled even if there is no
link, which might sometimes be useful.

-michael

Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock

[Jakub Kicinski]

On Wed, 11 May 2022 14:58:55 +0200 Michael Walle wrote:
> > I'm still not convinced that exposing the free running vs recovered
> > distinction from the start is a good idea. Intuitively it'd seem that
> > it's better to use the recovered clock to feed the wire side of the MAC,
> > this patch set uses the free running. So I'd personally strip the last 
> > part off and add it later if needed.  
> 
> FWIW, the recovered clock only works if there is a link. AFAIR on the
> AR8031 you can have the free-running one enabled even if there is no
> link, which might sometimes be useful.

Is that true for all PHYs? I've seen "larger" devices mention holdover
or some other form of automatic fallback in the DPLL if input clock is
lost. I thought that's the case here, too:

> > > +      The phy can also automatically switch between the reference and the
> > > +      respective 125MHz clocks based on its internal state.

Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock

[Michael Walle]

Am 2022-05-11 18:11, schrieb Jakub Kicinski:
> On Wed, 11 May 2022 14:58:55 +0200 Michael Walle wrote:
>> > I'm still not convinced that exposing the free running vs recovered
>> > distinction from the start is a good idea. Intuitively it'd seem that
>> > it's better to use the recovered clock to feed the wire side of the MAC,
>> > this patch set uses the free running. So I'd personally strip the last
>> > part off and add it later if needed.
>> 
>> FWIW, the recovered clock only works if there is a link. AFAIR on the
>> AR8031 you can have the free-running one enabled even if there is no
>> link, which might sometimes be useful.
> 
> Is that true for all PHYs? I've seen "larger" devices mention holdover
> or some other form of automatic fallback in the DPLL if input clock is
> lost.

I certainly can't speak of 'all' PHYs, who can ;) But how is the
switchover for example? hitless? will there be a brief period of
no clock at all?

The point I wanted to add is that the user should have the choice or
at least you should clearly mention that. If you drop the suffix and 
just
use "25mhz" is that now the recovered one or the free-running one. And
why is that one preferred over the other? Eg. if I were a designer for a
cheapo board and I'd need a 125MHz clock and want to save some bucks
for the 125MHz osc and the PHY could supply one, I'd use the
free-running mode. Just to avoid any surprises with a switchover
or whatever.

> I thought that's the case here, too:
> 
>> > > +      The phy can also automatically switch between the reference and the
>> > > +      respective 125MHz clocks based on its internal state.

I read "reference" as being the 25MHz (maybe when the PHY is in 10Mbit
mode? I didn't read the datasheet) because the first mode is called
25mhz-reference. So it might be switching between 25MHz and 125MHz?
I don't know.

-michael

Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock

[Jakub Kicinski]

On Wed, 11 May 2022 19:10:36 +0200 Michael Walle wrote:
> Am 2022-05-11 18:11, schrieb Jakub Kicinski:
> > On Wed, 11 May 2022 14:58:55 +0200 Michael Walle wrote:  
> >> FWIW, the recovered clock only works if there is a link. AFAIR on the
> >> AR8031 you can have the free-running one enabled even if there is no
> >> link, which might sometimes be useful.  
> > 
> > Is that true for all PHYs? I've seen "larger" devices mention holdover
> > or some other form of automatic fallback in the DPLL if input clock is
> > lost.  
> 
> I certainly can't speak of 'all' PHYs, who can ;) But how is the
> switchover for example? hitless? will there be a brief period of
> no clock at all?
> 
> The point I wanted to add is that the user should have the choice or
> at least you should clearly mention that. If you drop the suffix and 
> just
> use "25mhz" is that now the recovered one or the free-running one. And
> why is that one preferred over the other? Eg. if I were a designer for a
> cheapo board and I'd need a 125MHz clock and want to save some bucks
> for the 125MHz osc and the PHY could supply one, I'd use the
> free-running mode. Just to avoid any surprises with a switchover
> or whatever.

It's pure speculation on my side. I don't even know if PHYs use 
the recovered clock to clock its output towards the MAC or that's 
a different clock domain.

My concern is that people will start to use DT to configure SyncE which
is entirely a runtime-controllable thing, and doesn't belong. Hence
my preference to hide the recovered vs free-running detail if we can
pick one that makes most sense for now.

Again, if you feel strongly that the current design is indeed needed 
to configure pure SOC<>PHY / MAC<>PHY clocking, just send a review tag
and I'll apply :)

> > I thought that's the case here, too:
> 
> I read "reference" as being the 25MHz (maybe when the PHY is in 10Mbit
> mode? I didn't read the datasheet) because the first mode is called
> 25mhz-reference. So it might be switching between 25MHz and 125MHz?
> I don't know.

I couldn't grok that from the datasheet. Anyway, it'd be good to make
it clearer that the second sentence refers to the "adaptive" mode and
not the behavior of the recovered clock when lock is lost. Just put
(adaptive) in the sentence somewhere.

Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock

[Michael Walle]

Am 2022-05-11 21:42, schrieb Jakub Kicinski:
> On Wed, 11 May 2022 19:10:36 +0200 Michael Walle wrote:
>> Am 2022-05-11 18:11, schrieb Jakub Kicinski:
>> > On Wed, 11 May 2022 14:58:55 +0200 Michael Walle wrote:
>> >> FWIW, the recovered clock only works if there is a link. AFAIR on the
>> >> AR8031 you can have the free-running one enabled even if there is no
>> >> link, which might sometimes be useful.
>> >
>> > Is that true for all PHYs? I've seen "larger" devices mention holdover
>> > or some other form of automatic fallback in the DPLL if input clock is
>> > lost.
>> 
>> I certainly can't speak of 'all' PHYs, who can ;) But how is the
>> switchover for example? hitless? will there be a brief period of
>> no clock at all?
>> 
>> The point I wanted to add is that the user should have the choice or
>> at least you should clearly mention that. If you drop the suffix and
>> just
>> use "25mhz" is that now the recovered one or the free-running one. And
>> why is that one preferred over the other? Eg. if I were a designer for 
>> a
>> cheapo board and I'd need a 125MHz clock and want to save some bucks
>> for the 125MHz osc and the PHY could supply one, I'd use the
>> free-running mode. Just to avoid any surprises with a switchover
>> or whatever.
> 
> It's pure speculation on my side. I don't even know if PHYs use
> the recovered clock to clock its output towards the MAC or that's
> a different clock domain.
> 
> My concern is that people will start to use DT to configure SyncE which
> is entirely a runtime-controllable thing, and doesn't belong. Hence
> my preference to hide the recovered vs free-running detail if we can
> pick one that makes most sense for now.

I see. That makes sense, but then wouldn't it make more sense to pick
the (simple) free-running one? As for SyncE you'd need the recovered
clock.

> Again, if you feel strongly that the current design is indeed needed
> to configure pure SOC<>PHY / MAC<>PHY clocking, just send a review tag
> and I'll apply :)

I just wanted to add my two cents :) I'm fine with either one.

>> > I thought that's the case here, too:
>> 
>> I read "reference" as being the 25MHz (maybe when the PHY is in 10Mbit
>> mode? I didn't read the datasheet) because the first mode is called
>> 25mhz-reference. So it might be switching between 25MHz and 125MHz?
>> I don't know.
> 
> I couldn't grok that from the datasheet. Anyway, it'd be good to make
> it clearer that the second sentence refers to the "adaptive" mode and
> not the behavior of the recovered clock when lock is lost. Just put
> (adaptive) in the sentence somewhere.

Mh, there is not much there, whatever "heartbeat clock" means.

-michael

Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock

[Jakub Kicinski]

On Thu, 12 May 2022 23:20:18 +0200 Michael Walle wrote:
> > It's pure speculation on my side. I don't even know if PHYs use
> > the recovered clock to clock its output towards the MAC or that's
> > a different clock domain.
> > 
> > My concern is that people will start to use DT to configure SyncE which
> > is entirely a runtime-controllable thing, and doesn't belong. Hence
> > my preference to hide the recovered vs free-running detail if we can
> > pick one that makes most sense for now.  
> 
> I see. That makes sense, but then wouldn't it make more sense to pick
> the (simple) free-running one? As for SyncE you'd need the recovered
> clock.

Sounds good.

Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock

[Josua Mayer]

\o/

I am not sure I can follow your conversation here ...

Am 13.05.22 um 01:44 schrieb Jakub Kicinski:
> On Thu, 12 May 2022 23:20:18 +0200 Michael Walle wrote:
>>> It's pure speculation on my side. I don't even know if PHYs use
>>> the recovered clock to clock its output towards the MAC or that's
>>> a different clock domain.
>>>
>>> My concern is that people will start to use DT to configure SyncE which
>>> is entirely a runtime-controllable thing, and doesn't belong.
Okay.
However phy drivers do not seem to implement runtime control of those 
clock output pins currently, so they are configured once in DT.
>>> Hence
>>> my preference to hide the recovered vs free-running detail if we can
>>> pick one that makes most sense for now.
I am not a fan of hiding information. The clock configuration register 
clearly supports this distinction.

Is this a political stance to say users may not "accidentally" enable 
SyncE by patching DT?
If so we should print a warning message when someone selects it?
>>
>> I see. That makes sense, but then wouldn't it make more sense to pick
>> the (simple) free-running one? As for SyncE you'd need the recovered
>> clock.
> 
> Sounds good.

Yep, it seems recovered clock is only for SyncE - and only if there is a 
master clock on the network. Sadly however documentation is sparse and I 
do not know if the adi phys would fall back to using their internal 
clock, or just refuse to operate at all.

Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock

[Jakub Kicinski]

On Sun, 15 May 2022 10:16:47 +0300 Josua Mayer wrote:
> Am 13.05.22 um 01:44 schrieb Jakub Kicinski:
> > On Thu, 12 May 2022 23:20:18 +0200 Michael Walle wrote:  
> >>> It's pure speculation on my side. I don't even know if PHYs use
> >>> the recovered clock to clock its output towards the MAC or that's
> >>> a different clock domain.
> >>>
> >>> My concern is that people will start to use DT to configure SyncE which
> >>> is entirely a runtime-controllable thing, and doesn't belong.  
> Okay.
> However phy drivers do not seem to implement runtime control of those 
> clock output pins currently, so they are configured once in DT.

To me that means nobody needs the recovered clock.

> >>> Hence
> >>> my preference to hide the recovered vs free-running detail if we can
> >>> pick one that makes most sense for now.  
> I am not a fan of hiding information. The clock configuration register 
> clearly supports this distinction.

Unless you expose all registers as a direct API to the user you'll be
"hiding information". I don't think we are exposing all possible
registers for this PHY, the two bits in question are no different.

> Is this a political stance to say users may not "accidentally" enable 
> SyncE by patching DT?
> If so we should print a warning message when someone selects it?

Why would we add a feature and then print a warning? We can always add 
the support later, once we have a use case for it.

> >> I see. That makes sense, but then wouldn't it make more sense to pick
> >> the (simple) free-running one? As for SyncE you'd need the recovered
> >> clock.  
> > 
> > Sounds good.  
> 
> Yep, it seems recovered clock is only for SyncE - and only if there is a 
> master clock on the network. Sadly however documentation is sparse and I 
> do not know if the adi phys would fall back to using their internal 
> clock, or just refuse to operate at all.

Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock

[Josua Mayer]

Am 16.05.22 um 20:43 schrieb Jakub Kicinski:
> On Sun, 15 May 2022 10:16:47 +0300 Josua Mayer wrote:
>> Am 13.05.22 um 01:44 schrieb Jakub Kicinski:
>>> On Thu, 12 May 2022 23:20:18 +0200 Michael Walle wrote:
>>>>> It's pure speculation on my side. I don't even know if PHYs use
>>>>> the recovered clock to clock its output towards the MAC or that's
>>>>> a different clock domain.
>>>>>
>>>>> My concern is that people will start to use DT to configure SyncE which
>>>>> is entirely a runtime-controllable thing, and doesn't belong.
>> Okay.
>> However phy drivers do not seem to implement runtime control of those
>> clock output pins currently, so they are configured once in DT.
> To me that means nobody needs the recovered clock.
Doesn't need it, or is overwhelmed by the idea of figuring out how to 
implement it properly.
>>>>> Hence
>>>>> my preference to hide the recovered vs free-running detail if we can
>>>>> pick one that makes most sense for now.
>> I am not a fan of hiding information. The clock configuration register
>> clearly supports this distinction.
> Unless you expose all registers as a direct API to the user you'll be
> "hiding information". I don't think we are exposing all possible
> registers for this PHY, the two bits in question are no different.
>
>> Is this a political stance to say users may not "accidentally" enable
>> SyncE by patching DT?
>> If so we should print a warning message when someone selects it?
> Why would we add a feature and then print a warning? We can always add
> the support later, once we have a use case for it.
I would not call it a feature.
We can e.g. not print a warning, and instead put in the DT binding a 
note that the recovered variants are for SyncE which Linux does not 
support.

As to why we would add the -recovered options,
for starters this allows curious developers to search for the term to 
get an idea which PHYs would technically support it.
That it would also allow tinkering with SyncE to me is a plus, but for 
you clearly a minus, and I can not make a strong case.

So I can imagine to change the bindings as follows:
1. remove the -recovered variants
2. add an explicit note in the commit message that the recovered clock 
is not implemented because we do not have infrastructure for SyncE
3. keep the -free-running suffix, we should imo only hide it on the day 
SyncE can be toggled by another means.

> I see. That makes sense, but then wouldn't it make more sense to pick
> the (simple) free-running one? As for SyncE you'd need the recovered
> clock.
>>> Sounds good.
>> Yep, it seems recovered clock is only for SyncE - and only if there is a
>> master clock on the network. Sadly however documentation is sparse and I
>> do not know if the adi phys would fall back to using their internal
>> clock, or just refuse to operate at all.

Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock

[Jakub Kicinski]

On Mon, 16 May 2022 22:48:20 +0300 Josua Mayer wrote:
> So I can imagine to change the bindings as follows:
> 1. remove the -recovered variants
> 2. add an explicit note in the commit message that the recovered clock 
> is not implemented because we do not have infrastructure for SyncE
> 3. keep the -free-running suffix, we should imo only hide it on the day 
> SyncE can be toggled by another means.

SGTM, thanks!

Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock

[Josua Mayer]

Am 17.05.22 um 01:40 schrieb Jakub Kicinski:
> On Mon, 16 May 2022 22:48:20 +0300 Josua Mayer wrote:
>> So I can imagine to change the bindings as follows:
>> 1. remove the -recovered variants
>> 2. add an explicit note in the commit message that the recovered clock
>> is not implemented because we do not have infrastructure for SyncE
>> 3. keep the -free-running suffix, we should imo only hide it on the day
>> SyncE can be toggled by another means.
> 
> SGTM, thanks!

Thank you for your comments, I am sending v5 shortly!