net: phy: adin: add missing clock option

[PATCH] net: phy: adin: add missing clock option

Posted by Fabian Pfitzner 1 year, 11 months ago

The GP_CLK pin on Adin1300 PHY's offers three different output clocks.
This patch adds the missing 125MHz recovered clock option which is not
yet availible in the driver.

Signed-off-by: Fabian Pfitzner <f.pfitzner@pengutronix.de>
---
 Documentation/devicetree/bindings/net/adi,adin.yaml | 7 +++++--
 drivers/net/phy/adin.c                              | 2 ++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
index 929cf8c0b0fd..cd1b4efa692b 100644
--- a/Documentation/devicetree/bindings/net/adi,adin.yaml
+++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
@@ -38,14 +38,17 @@ properties:
 
   adi,phy-output-clock:
     description: |
-      Select clock output on GP_CLK pin. Two clocks are available:
-      A 25MHz reference and a free-running 125MHz.
+      Select clock output on GP_CLK pin. Three clocks are available:
+        - 25MHz reference
+        - free-running 125MHz 
+        - recovered 125MHz
       The phy can alternatively automatically switch between the reference and
       the 125MHz clocks based on its internal state.
     $ref: /schemas/types.yaml#/definitions/string
     enum:
       - 25mhz-reference
       - 125mhz-free-running
+      - 125mhz-recovered
       - adaptive-free-running
 
   adi,phy-output-reference-clock:
diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 2e1a46e121d9..b1ed6fd24763 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -508,6 +508,8 @@ static int adin_config_clk_out(struct phy_device *phydev)
 		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 {

base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
-- 
2.39.2

Re: [PATCH] net: phy: adin: add missing clock option

Posted by Krzysztof Kozlowski 1 year, 11 months ago

On 22/01/2024 12:03, Fabian Pfitzner wrote:
> The GP_CLK pin on Adin1300 PHY's offers three different output clocks.
> This patch adds the missing 125MHz recovered clock option which is not
> yet availible in the driver.
> 

Typo

> Signed-off-by: Fabian Pfitzner <f.pfitzner@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/net/adi,adin.yaml | 7 +++++--

Bindings are separate.

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

Best regards,
Krzysztof

Re: [PATCH] net: phy: adin: add missing clock option

Posted by Simon Horman 1 year, 11 months ago

On Mon, Jan 22, 2024 at 12:03:12PM +0100, Fabian Pfitzner wrote:
> The GP_CLK pin on Adin1300 PHY's offers three different output clocks.
> This patch adds the missing 125MHz recovered clock option which is not
> yet availible in the driver.

nit: available

...

Re: [PATCH] net: phy: adin: add missing clock option

Posted by Conor Dooley 1 year, 11 months ago

On Mon, Jan 22, 2024 at 12:03:12PM +0100, Fabian Pfitzner wrote:
> The GP_CLK pin on Adin1300 PHY's offers three different output clocks.
> This patch adds the missing 125MHz recovered clock option which is not
> yet availible in the driver.
> 
> Signed-off-by: Fabian Pfitzner <f.pfitzner@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/net/adi,adin.yaml | 7 +++++--

Binding patches should be split out from drivers please.

Thanks,
Conor.

>  drivers/net/phy/adin.c                              | 2 ++
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
> index 929cf8c0b0fd..cd1b4efa692b 100644
> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml
> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
> @@ -38,14 +38,17 @@ properties:
>  
>    adi,phy-output-clock:
>      description: |
> -      Select clock output on GP_CLK pin. Two clocks are available:
> -      A 25MHz reference and a free-running 125MHz.
> +      Select clock output on GP_CLK pin. Three clocks are available:
> +        - 25MHz reference
> +        - free-running 125MHz 
> +        - recovered 125MHz
>        The phy can alternatively automatically switch between the reference and
>        the 125MHz clocks based on its internal state.
>      $ref: /schemas/types.yaml#/definitions/string
>      enum:
>        - 25mhz-reference
>        - 125mhz-free-running
> +      - 125mhz-recovered
>        - adaptive-free-running
>  
>    adi,phy-output-reference-clock:
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index 2e1a46e121d9..b1ed6fd24763 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -508,6 +508,8 @@ static int adin_config_clk_out(struct phy_device *phydev)
>  		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 {
> 
> base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
> -- 
> 2.39.2
>

[PATCH v2 1/2] dt-bindings: net: adin: add recovered clock output

Posted by Fabian Pfitzner 1 year, 11 months ago

The ADIN1300 offers three distinct output clocks which can be accessed
through the GP_CLK pin. The DT only offers two of the possible options
and thus the 125MHz-recovered output clock is missing.

As there is no other way to configure this pin than through the DT it
should be possible to do so for all available outputs.

Signed-off-by: Fabian Pfitzner <f.pfitzner@pengutronix.de>
---
 Documentation/devicetree/bindings/net/adi,adin.yaml | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
index 929cf8c0b0fd..04059393b756 100644
--- a/Documentation/devicetree/bindings/net/adi,adin.yaml
+++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
@@ -38,14 +38,17 @@ properties:
 
   adi,phy-output-clock:
     description: |
-      Select clock output on GP_CLK pin. Two clocks are available:
-      A 25MHz reference and a free-running 125MHz.
+      Select clock output on GP_CLK pin. Three clocks are available:
+        - 25MHz reference
+        - free-running 125MHz
+        - recovered 125MHz
       The phy can alternatively automatically switch between the reference and
       the 125MHz clocks based on its internal state.
     $ref: /schemas/types.yaml#/definitions/string
     enum:
       - 25mhz-reference
       - 125mhz-free-running
+      - 125mhz-recovered
       - adaptive-free-running
 
   adi,phy-output-reference-clock:

base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
-- 
2.39.2

Re: [PATCH v2 1/2] dt-bindings: net: adin: add recovered clock output

Posted by Jakub Kicinski 1 year, 11 months ago

On Wed, 24 Jan 2024 11:25:54 +0100 Fabian Pfitzner wrote:
> The ADIN1300 offers three distinct output clocks which can be accessed
> through the GP_CLK pin. The DT only offers two of the possible options
> and thus the 125MHz-recovered output clock is missing.
> 
> As there is no other way to configure this pin than through the DT it
> should be possible to do so for all available outputs.

Hi Fabian!

If you want to use PHY-recovered clock you should really use the DPLL
subsystem. It will also allow you to configure other PHYs taking this
signal as an input, to forward the clock phase. Read lock state. Etc.

Even if the patches are good (which I'm not saying they are yet ;)) -
you'll have to repost this as a new thread, unfortunately. I'm not sure
why by the way this was posted made patchwork think that the patches
are separate series:

https://patchwork.kernel.org/project/netdevbpf/list/?series=819440
https://patchwork.kernel.org/project/netdevbpf/list/?series=818548

each of which is incomplete, since it only has one patch but subject
says "1/2" and "2/2".

Re: [PATCH v2 1/2] dt-bindings: net: adin: add recovered clock output

Posted by Conor Dooley 1 year, 11 months ago

On Wed, Jan 24, 2024 at 11:25:54AM +0100, Fabian Pfitzner wrote:
> The ADIN1300 offers three distinct output clocks which can be accessed
> through the GP_CLK pin. The DT only offers two of the possible options
> and thus the 125MHz-recovered output clock is missing.
> 
> As there is no other way to configure this pin than through the DT it
> should be possible to do so for all available outputs.
> 
> Signed-off-by: Fabian Pfitzner <f.pfitzner@pengutronix.de>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> ---
>  Documentation/devicetree/bindings/net/adi,adin.yaml | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
> index 929cf8c0b0fd..04059393b756 100644
> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml
> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
> @@ -38,14 +38,17 @@ properties:
>  
>    adi,phy-output-clock:
>      description: |
> -      Select clock output on GP_CLK pin. Two clocks are available:
> -      A 25MHz reference and a free-running 125MHz.
> +      Select clock output on GP_CLK pin. Three clocks are available:
> +        - 25MHz reference
> +        - free-running 125MHz
> +        - recovered 125MHz
>        The phy can alternatively automatically switch between the reference and
>        the 125MHz clocks based on its internal state.
>      $ref: /schemas/types.yaml#/definitions/string
>      enum:
>        - 25mhz-reference
>        - 125mhz-free-running
> +      - 125mhz-recovered
>        - adaptive-free-running
>  
>    adi,phy-output-reference-clock:
> 
> base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
> -- 
> 2.39.2
>

[PATCH v2 2/2] net: phy: adin: add recovered clock output

Posted by Fabian Pfitzner 1 year, 11 months ago

The ADIN1300 offers three distinct output clocks which can be accessed
through the GP_CLK pin. The DT only offers two of the possible options
and thus the 125MHz-recovered output clock is missing.

As there is no other way to configure this pin than through the DT it
should be possible to do so for all available outputs.

Signed-off-by: Fabian Pfitzner <f.pfitzner@pengutronix.de>
---
 drivers/net/phy/adin.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 2e1a46e121d9..b1ed6fd24763 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -508,6 +508,8 @@ static int adin_config_clk_out(struct phy_device *phydev)
 		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 {
-- 
2.39.2

Re: [PATCH v2 2/2] net: phy: adin: add recovered clock output

Posted by Nuno Sá 1 year, 11 months ago

On Wed, 2024-01-24 at 11:25 +0100, Fabian Pfitzner wrote:
> The ADIN1300 offers three distinct output clocks which can be accessed
> through the GP_CLK pin. The DT only offers two of the possible options
> and thus the 125MHz-recovered output clock is missing.
> 
> As there is no other way to configure this pin than through the DT it
> should be possible to do so for all available outputs.
> 
> Signed-off-by: Fabian Pfitzner <f.pfitzner@pengutronix.de>
> ---

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/net/phy/adin.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index 2e1a46e121d9..b1ed6fd24763 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -508,6 +508,8 @@ static int adin_config_clk_out(struct phy_device *phydev)
>  		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 {