Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <cfbde0da-9939-e976-52c1-88577de7d4cb@linaro.org>
Date:   Sun, 23 Oct 2022 11:41:42 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.0
Subject: Re: [PATCH net-next V2] dt-bindings: net: ethernet-controller: Add
 ptp-hardware-clock
Content-Language: en-US
To:     Sarath Babu Naidu Gaddam <sarath.babu.naidu.gaddam@amd.com>,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, robh+dt@kernel.org, richardcochran@gmail.com
Cc:     krzysztof.kozlowski+dt@linaro.org, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        yangbo.lu@nxp.com, radhey.shyam.pandey@amd.com,
        anirudha.sarangi@amd.com, harini.katakam@amd.com, git@amd.com
References: <20221021054111.25852-1-sarath.babu.naidu.gaddam@amd.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20221021054111.25852-1-sarath.babu.naidu.gaddam@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 21/10/2022 01:41, Sarath Babu Naidu Gaddam wrote:
> There is currently no standard property to pass PTP device index
> information to ethernet driver when they are independent.
> 
> ptp-hardware-clock property will contain phandle to PTP clock node.
> 
> Freescale driver currently has this implementation but it will be
> good to agree on a generic (optional) property name to link to PTP
> phandle to Ethernet node. In future or any current ethernet driver
> wants to use this method of reading the PHC index,they can simply use
> this generic name and point their own PTP clock node, instead of
> creating separate property names in each ethernet driver DT node.
> 
> axiethernet driver uses this method when PTP support is integrated.
> 
> Example:
> 	fman0: fman@1a00000 {
> 		ptp-hardware-clock = <&ptp_timer0>;
> 	}
> 
> 	ptp_timer0: ptp-timer@1afe000 {
> 		compatible = "fsl,fman-ptp-timer";
> 		reg = <0x0 0x1afe000 0x0 0x1000>;
> 	}
> 
> Signed-off-by: Sarath Babu Naidu Gaddam <sarath.babu.naidu.gaddam@amd.com>
> ---
> We want binding to be reviewed/accepted and then make changes in freescale
> binding documentation to use this generic binding.

No, send entire set. We need to see the users of it.

> 
> DT information:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> tree/arch/arm64/boot/dts/freescale/qoriq-fman3-0.dtsi#n23

Don't wrap links. It's not possible to click them...

> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> tree/Documentation/devicetree/bindings/net/fsl-fman.txt#n320
> 
> Freescale driver:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> tree/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c#n467
> 
> Changes in V2:
> 1) Changed the ptimer-handle to ptp-hardware-clock based on
>    Richard Cochran's comment.
> 2) Updated commit description.
> ---
>  .../devicetree/bindings/net/ethernet-controller.yaml         | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> index 3aef506fa158..d2863c1dd585 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -161,6 +161,11 @@ properties:
>        - auto
>        - in-band-status
>  
> +  ptp-hardware-clock:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Specifies a reference to a node representing a IEEE1588 timer.

Drop "Specifies a reference to". It's obvious from the schema.

Aren't you expecting here some specific Devicetree node of IEEE1588
timer? IOW, you expect to point to timer, but what this timer must
provide? How is this generic?

In your commit msg you use multiple times "driver", so are you adding it
only to satisfy Linux driver requirements? What about other drivers,
e.g. on BSD or U-Boot?

Best regards,
Krzysztof