Received-SPF: pass (google.com: domain of linux-kernel+bounces-19606-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 8 Jan 2024 14:19:52 +0100
From: Andrew Lunn <andrew@lunn.ch>
To: Jie Luo <quic_luoj@quicinc.com>
Cc: Sergey Ryazanov <ryazanov.s.a@gmail.com>,
	Christian Marangi <ansuelsmth@gmail.com>,
	Robert Marko <robert.marko@sartura.hr>,
	Vladimir Oltean <olteanv@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>, Andy Gross <agross@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>,
	netdev@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet
 PHY package nodes
Message-ID: <841ef784-b27e-4f7a-94f2-f04f93178c61@lunn.ch>
References: <20231126015346.25208-1-ansuelsmth@gmail.com>
 <20231126015346.25208-2-ansuelsmth@gmail.com>
 <0926ea46-1ce4-4118-a04c-b6badc0b9e15@gmail.com>
 <1437d9df-2868-43f5-aebd-e0c57fe4d905@lunn.ch>
 <b75e6267-7d54-412e-8882-af4d9a0b54e6@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b75e6267-7d54-412e-8882-af4d9a0b54e6@quicinc.com>

> Since qca8075 PHY is also multiple port PHY, which is same as qca8084,
> but qca8084 also includes the integrated clock controller, this is the
> first qcom PHY chip integrating the clock controller internally.
> can we also consider designing the clocks and resets DT models in the
> PHY package DT.
> 
> For qca8084 PURE PHY chip, which is the quad PHY chip and two PCSes,
> it integrates the clock controller that generates the clocks to be used
> by the link of PHYs, the integrated controller also provides the resets
> to the PHY,  the clock controller(NSSCC) driver of qca8084 works at the
> same way of the GCC of SoC(IPQ), qca8084 needs to be initialized with
> the clocks and resets for the qca8084 PHY package, these clocks and
> resets are generated by the NSSCC, even for PURE phy chip qca8084, there
> is also some PHY package level clocks needs to be initialized.
> 
> here is the diagram of qca8084.
> __| |_______________| |__
> | PCS0 |          |PCS1 |
> |______|          |_____|
> |_________________      |
> |                |      |
> |     NSSCC      |      |
> |________________|      |
> |_______________________|
> |     |     |     |     |
> |PHY1 |PHY2 |PHY3 |PHY4 |
> |_____|_____|_____|_____|

Please add to the diagram the external clocks and external resets.

Additionally, add the resets and clocks between the NSSCC and the
individual PHYs. Typically, the internal clocks and resets are not in
DT, at last not for a single PHY. For a quad PHY in a package, it
might make sense to add them. Before we can decide that, we need a
clear idea what the hardware looks like.

> let me example the initial clocks and resets for the pure PHY chip qca8084
> as below, the clocks and resets should be put into the first
> MDIO node to be initialized firstly before qca8084 PHY will work.
> 
> ethernet-phy-package@0 {
> 
>         #address-cells = <1>;
> 
>         #size-cells = <0>;
> 
>         compatible = "ethernet-phy-package";
> 
>         reg = <0>;
> 
> 
> 
>         /* initial PHY package level clocks */
> 
>         clocks = <&qca8k_nsscc NSS_CC_APB_BRIDGE_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_AHB_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_SEC_CTRL_AHB_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_TLMM_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_TLMM_AHB_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_CNOC_AHB_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_MDIO_AHB_CLK>;

Device tree effectively defined devices on bus, in a tree, and how
they interconnect. Does the NSSCC have its own address on the MDIO
bus? Or does it share an address with one of the PHYs? It could be we
want to describe the NSSCC as a DT node of its own within the
package. It is probably both a clock consumer, and a clock provider.
The individual PHYs are then clock consumers, of the clocks the NSSCC
exports. Same for resets.

> 
>         clock-names = "apb_bridge",
> 
>                 "ahb",
> 
>                 "sec_ctrl_ahb",
> 
>                 "tlmm",
> 
>                 "tlmm_ahb",
> 
>                 "cnoc_ahb",
> 
>                 "mdio_ahb";
> 
> 
> 
>         /* initial PHY package level reset */
> 
>         resets = <&qca8k_nsscc NSS_CC_DSP_ARES>;
> 
>         reset-names = "gephy_dsp";
> 
> 
> 
>         /* initial clocks and resets for first phy */
> 
>         phy0 {
> 
>                 reg = <0>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>;
> 
>                 clock-names = "gephy0_sys";
> 
>                 resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>,
> 
>                        <&qca8k_nsscc NSS_CC_GEPHY0_ARES>;
> 
>                 reset-names = "gephy0_sys",
> 
>                         "gephy0_soft";
> 
>         };
> 
> 
> 
>         /* initial clocks and resets for second phy */
> 
>         phy1 {
> 
>                 reg = <1>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_CLK>;
> 
>                 clock-names = "gephy1_sys";
> 
>                 resets = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_ARES>,
> 
>                        <&qca8k_nsscc NSS_CC_GEPHY1_ARES>;
> 
>                 reset-names = "gephy1_sys",
> 
>                         "gephy1_soft";
> 
>         };
> 
> 
> 
>         /* initial clocks and resets for third phy */
> 
>         phy2 {
> 
>                 reg = <2>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_CLK>;
> 
>                 clock-names = "gephy2_sys";
>                 resets = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_ARES>,
> 
>                        <&qca8k_nsscc NSS_CC_GEPHY2_ARES>;
> 
>                 reset-names = "gephy2_sys",
> 
>                         "gephy2_soft";
> 
>         };
> 
> 
> 
>         /* initial clocks and resets for fourth phy */
> 
>         phy3 {
> 
>                 reg = <3>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_CLK>;
> 
>                 clock-names = "gephy3_sys";
> 
>                 resets = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_ARES>,
> 
>                        <&qca8k_nsscc NSS_CC_GEPHY3_ARES>;
> 
>                 reset-names = "gephy3_sys",
> 
>                         "gephy3_soft";
> 
>         };

This is starting to look O.K.

>         /* initial clocks and resets for pcs0. */
> 
>         pcs0 {
> 
>                 reg = <4>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_SRDS0_SYS_CLK>;
> 
>                 clock-names = "srds0_sys";
> 
>                 resets = <&qca8k_nsscc NSS_CC_SRDS0_SYS_ARES>;
> 
>                 reset-names = "srds0_sys";
> 
>         };
> 
> 
> 
>         /* initial clocks and resets for pcs1. */
> 
>         pcs1 {
> 
>                 reg = <5>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_SRDS1_SYS_CLK>;
> 
>                 clock-names = "srds1_sys";
> 
>                 resets = <&qca8k_nsscc NSS_CC_SRDS1_SYS_ARES>;
> 
>                 reset-names = "srds1_sys";
> 
>         };

PCS will need further work and thinking about. Typically, they are not
described in DT for a PHY. In general, a PCS in a PHY does not have a
driver of its own, the firmware in the PHY mostly controls it, not
Linux. For the moment, lets leave them as they are, and we will come
back to them once we get the clocks and resets correctly described.

     Andrew