[v5,3/5] arm64: dts: ti: k3-j784s4: Add WIZ and SERDES PHY nodes

Message ID 20230710101705.154119-4-j-choudhary@ti.com
State New
Headers
Series Add peripherals for J784S4 |

Commit Message

Jayesh Choudhary July 10, 2023, 10:17 a.m. UTC
  From: Siddharth Vadapalli <s-vadapalli@ti.com>

J784S4 SoC has 4 Serdes instances along with their respective WIZ
instances. Add device-tree nodes for them and disable them by default.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
[j-choudhary@ti.com: fix serdes_wiz clock order]
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---
 arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi | 171 +++++++++++++++++++++
 1 file changed, 171 insertions(+)
  

Comments

Nishanth Menon July 12, 2023, 2:18 p.m. UTC | #1
On 15:47-20230710, Jayesh Choudhary wrote:
> From: Siddharth Vadapalli <s-vadapalli@ti.com>
> 
> J784S4 SoC has 4 Serdes instances along with their respective WIZ
> instances. Add device-tree nodes for them and disable them by default.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> [j-choudhary@ti.com: fix serdes_wiz clock order]
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> ---
NAK. This patch introduces the following dtbs_check warning.
arch/arm64/boot/dts/ti/k3-am69-sk.dtb: serdes-refclk: 'clock-frequency' is a required property
  
Jayesh Choudhary July 13, 2023, 3:31 p.m. UTC | #2
On 12/07/23 19:48, Nishanth Menon wrote:
> On 15:47-20230710, Jayesh Choudhary wrote:
>> From: Siddharth Vadapalli <s-vadapalli@ti.com>
>>
>> J784S4 SoC has 4 Serdes instances along with their respective WIZ
>> instances. Add device-tree nodes for them and disable them by default.
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> [j-choudhary@ti.com: fix serdes_wiz clock order]
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> ---
> NAK. This patch introduces the following dtbs_check warning.
> arch/arm64/boot/dts/ti/k3-am69-sk.dtb: serdes-refclk: 'clock-frequency' is a required property
> 

Sorry for this. This property was added in the final board file.
I will fix it in the next revision.
I will add '0' as clock-property in the main file similar to j721e[1]
which will be overridden in the board file with required value to get
rid of this warning.

Warm Regards,
-Jayesh

[1]: 
<https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi#n16>
  
Nishanth Menon July 13, 2023, 6:21 p.m. UTC | #3
On 21:01-20230713, Jayesh Choudhary wrote:
> 
> 
> On 12/07/23 19:48, Nishanth Menon wrote:
> > On 15:47-20230710, Jayesh Choudhary wrote:
> > > From: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > 
> > > J784S4 SoC has 4 Serdes instances along with their respective WIZ
> > > instances. Add device-tree nodes for them and disable them by default.
> > > 
> > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > [j-choudhary@ti.com: fix serdes_wiz clock order]
> > > Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> > > ---
> > NAK. This patch introduces the following dtbs_check warning.
> > arch/arm64/boot/dts/ti/k3-am69-sk.dtb: serdes-refclk: 'clock-frequency' is a required property
> > 
> 
> Sorry for this. This property was added in the final board file.
> I will fix it in the next revision.
> I will add '0' as clock-property in the main file similar to j721e[1]
> which will be overridden in the board file with required value to get
> rid of this warning.

That would follow what renesas (r8a774a1.dtsi) and imx
(imx8dxl-ss-conn.dtsi) seem to be doing as well. Just make sure to add
documentation to the property to indicate expectation. Unless someone
has objections to this approach.
  
Andrew Davis July 13, 2023, 6:31 p.m. UTC | #4
On 7/13/23 1:21 PM, Nishanth Menon wrote:
> On 21:01-20230713, Jayesh Choudhary wrote:
>>
>>
>> On 12/07/23 19:48, Nishanth Menon wrote:
>>> On 15:47-20230710, Jayesh Choudhary wrote:
>>>> From: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>>
>>>> J784S4 SoC has 4 Serdes instances along with their respective WIZ
>>>> instances. Add device-tree nodes for them and disable them by default.
>>>>
>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>> [j-choudhary@ti.com: fix serdes_wiz clock order]
>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>> ---
>>> NAK. This patch introduces the following dtbs_check warning.
>>> arch/arm64/boot/dts/ti/k3-am69-sk.dtb: serdes-refclk: 'clock-frequency' is a required property
>>>
>>
>> Sorry for this. This property was added in the final board file.
>> I will fix it in the next revision.
>> I will add '0' as clock-property in the main file similar to j721e[1]
>> which will be overridden in the board file with required value to get
>> rid of this warning.
> 
> That would follow what renesas (r8a774a1.dtsi) and imx
> (imx8dxl-ss-conn.dtsi) seem to be doing as well. Just make sure to add
> documentation to the property to indicate expectation. Unless someone
> has objections to this approach.
> 

Would it work better to disable these nodes, only enabling them in the
board files when a real clock-frequency can be provided?

My initial reaction would be to move the whole external reference clock
node to the board file since that is where it is provided, but seems
that would cause more churn in serdes_wiz* nodes than we would want..

Andrew
  
Nishanth Menon July 13, 2023, 6:58 p.m. UTC | #5
On 13:31-20230713, Andrew Davis wrote:
> On 7/13/23 1:21 PM, Nishanth Menon wrote:
> > On 21:01-20230713, Jayesh Choudhary wrote:
> > > 
> > > 
> > > On 12/07/23 19:48, Nishanth Menon wrote:
> > > > On 15:47-20230710, Jayesh Choudhary wrote:
> > > > > From: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > > > 
> > > > > J784S4 SoC has 4 Serdes instances along with their respective WIZ
> > > > > instances. Add device-tree nodes for them and disable them by default.
> > > > > 
> > > > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > > > [j-choudhary@ti.com: fix serdes_wiz clock order]
> > > > > Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> > > > > ---
> > > > NAK. This patch introduces the following dtbs_check warning.
> > > > arch/arm64/boot/dts/ti/k3-am69-sk.dtb: serdes-refclk: 'clock-frequency' is a required property
> > > > 
> > > 
> > > Sorry for this. This property was added in the final board file.
> > > I will fix it in the next revision.
> > > I will add '0' as clock-property in the main file similar to j721e[1]
> > > which will be overridden in the board file with required value to get
> > > rid of this warning.
> > 
> > That would follow what renesas (r8a774a1.dtsi) and imx
> > (imx8dxl-ss-conn.dtsi) seem to be doing as well. Just make sure to add
> > documentation to the property to indicate expectation. Unless someone
> > has objections to this approach.
> > 
> 
> Would it work better to disable these nodes, only enabling them in the
> board files when a real clock-frequency can be provided?
> 
> My initial reaction would be to move the whole external reference clock
> node to the board file since that is where it is provided, but seems
> that would cause more churn in serdes_wiz* nodes than we would want..

I would prefer that as well, but I have'nt gone around looking for
similar examples on other SoCs (Jayesh, can you check?). One other
approach (alipine and few other places) has been for the bootloader to
update the property set in dtb as 0, which is not needed in this case
to the best of what I see.. just hoping we use a technique that most
board folks are familiar with across SoCs.
  
Jayesh Choudhary July 20, 2023, 9:36 a.m. UTC | #6
On 14/07/23 00:28, Nishanth Menon wrote:
> On 13:31-20230713, Andrew Davis wrote:
>> On 7/13/23 1:21 PM, Nishanth Menon wrote:
>>> On 21:01-20230713, Jayesh Choudhary wrote:
>>>>
>>>>
>>>> On 12/07/23 19:48, Nishanth Menon wrote:
>>>>> On 15:47-20230710, Jayesh Choudhary wrote:
>>>>>> From: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>>>>
>>>>>> J784S4 SoC has 4 Serdes instances along with their respective WIZ
>>>>>> instances. Add device-tree nodes for them and disable them by default.
>>>>>>
>>>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>>>> [j-choudhary@ti.com: fix serdes_wiz clock order]
>>>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>>>> ---
>>>>> NAK. This patch introduces the following dtbs_check warning.
>>>>> arch/arm64/boot/dts/ti/k3-am69-sk.dtb: serdes-refclk: 'clock-frequency' is a required property
>>>>>
>>>>
>>>> Sorry for this. This property was added in the final board file.
>>>> I will fix it in the next revision.
>>>> I will add '0' as clock-property in the main file similar to j721e[1]
>>>> which will be overridden in the board file with required value to get
>>>> rid of this warning.
>>>
>>> That would follow what renesas (r8a774a1.dtsi) and imx
>>> (imx8dxl-ss-conn.dtsi) seem to be doing as well. Just make sure to add
>>> documentation to the property to indicate expectation. Unless someone
>>> has objections to this approach.
>>>
>>
>> Would it work better to disable these nodes, only enabling them in the
>> board files when a real clock-frequency can be provided?
>>
>> My initial reaction would be to move the whole external reference clock
>> node to the board file since that is where it is provided, but seems
>> that would cause more churn in serdes_wiz* nodes than we would want..
> 
> I would prefer that as well, but I have'nt gone around looking for
> similar examples on other SoCs (Jayesh, can you check?). One other
> approach (alipine and few other places) has been for the bootloader to
> update the property set in dtb as 0, which is not needed in this case
> to the best of what I see.. just hoping we use a technique that most
> board folks are familiar with across SoCs.
> 


I can see the clock nodes in board files for some vendors. But like
Andrew said, that would cause issues with serdes_wiz node in the
main.dtsi.

So I think it would be better to keep the external clock node in main
dtsi itself. I will add comments to the property to indicate that this
value will be over-written in board dts.
Posting v6 to address these comments and others on the series.

Thanks,
Jayesh
  
Jayesh Choudhary July 21, 2023, 1:26 p.m. UTC | #7
On 20/07/23 15:06, Jayesh Choudhary wrote:
> 
> 
> On 14/07/23 00:28, Nishanth Menon wrote:
>> On 13:31-20230713, Andrew Davis wrote:
>>> On 7/13/23 1:21 PM, Nishanth Menon wrote:
>>>> On 21:01-20230713, Jayesh Choudhary wrote:
>>>>>
>>>>>
>>>>> On 12/07/23 19:48, Nishanth Menon wrote:
>>>>>> On 15:47-20230710, Jayesh Choudhary wrote:
>>>>>>> From: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>>>>>
>>>>>>> J784S4 SoC has 4 Serdes instances along with their respective WIZ
>>>>>>> instances. Add device-tree nodes for them and disable them by 
>>>>>>> default.
>>>>>>>
>>>>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>>>>> [j-choudhary@ti.com: fix serdes_wiz clock order]
>>>>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>>>>> ---
>>>>>> NAK. This patch introduces the following dtbs_check warning.
>>>>>> arch/arm64/boot/dts/ti/k3-am69-sk.dtb: serdes-refclk: 
>>>>>> 'clock-frequency' is a required property
>>>>>>
>>>>>
>>>>> Sorry for this. This property was added in the final board file.
>>>>> I will fix it in the next revision.
>>>>> I will add '0' as clock-property in the main file similar to j721e[1]
>>>>> which will be overridden in the board file with required value to get
>>>>> rid of this warning.
>>>>
>>>> That would follow what renesas (r8a774a1.dtsi) and imx
>>>> (imx8dxl-ss-conn.dtsi) seem to be doing as well. Just make sure to add
>>>> documentation to the property to indicate expectation. Unless someone
>>>> has objections to this approach.
>>>>
>>>
>>> Would it work better to disable these nodes, only enabling them in the
>>> board files when a real clock-frequency can be provided?
>>>
>>> My initial reaction would be to move the whole external reference clock
>>> node to the board file since that is where it is provided, but seems
>>> that would cause more churn in serdes_wiz* nodes than we would want..
>>
>> I would prefer that as well, but I have'nt gone around looking for
>> similar examples on other SoCs (Jayesh, can you check?). One other
>> approach (alipine and few other places) has been for the bootloader to
>> update the property set in dtb as 0, which is not needed in this case
>> to the best of what I see.. just hoping we use a technique that most
>> board folks are familiar with across SoCs.
>>
> 
> 
> I can see the clock nodes in board files for some vendors. But like
> Andrew said, that would cause issues with serdes_wiz node in the
> main.dtsi.
> 
> So I think it would be better to keep the external clock node in main
> dtsi itself. I will add comments to the property to indicate that this
> value will be over-written in board dts.
> Posting v6 to address these comments and others on the series.
> 

FYI..
I posted v6: 
https://lore.kernel.org/all/20230721132029.123881-1-j-choudhary@ti.com/
And I am using status property instead to address the dtbs_warning here.

Warm Regards,
Jayesh
  

Patch

diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
index baffa8428d64..151921f73848 100644
--- a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
@@ -7,6 +7,15 @@ 
 
 #include <dt-bindings/mux/mux.h>
 #include <dt-bindings/mux/ti-serdes.h>
+#include <dt-bindings/phy/phy.h>
+#include <dt-bindings/phy/phy-ti.h>
+
+/ {
+	serdes_refclk: serdes-refclk {
+		#clock-cells = <0>;
+		compatible = "fixed-clock";
+	};
+};
 
 &cbass_main {
 	msmc_ram: sram@70000000 {
@@ -698,6 +707,168 @@  main_sdhci1: mmc@4fb0000 {
 		status = "disabled";
 	};
 
+	serdes_wiz0: wiz@5060000 {
+		compatible = "ti,j784s4-wiz-10g";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		power-domains = <&k3_pds 404 TI_SCI_PD_EXCLUSIVE>;
+		clocks = <&k3_clks 404 2>, <&k3_clks 404 6>, <&serdes_refclk>, <&k3_clks 404 5>;
+		clock-names = "fck", "core_ref_clk", "ext_ref_clk", "core_ref1_clk";
+		assigned-clocks = <&k3_clks 404 6>;
+		assigned-clock-parents = <&k3_clks 404 10>;
+		num-lanes = <4>;
+		#reset-cells = <1>;
+		#clock-cells = <1>;
+		ranges = <0x5060000 0x00 0x5060000 0x10000>;
+
+		status = "disabled";
+
+		serdes0: serdes@5060000 {
+			compatible = "ti,j721e-serdes-10g";
+			reg = <0x05060000 0x010000>;
+			reg-names = "torrent_phy";
+			resets = <&serdes_wiz0 0>;
+			reset-names = "torrent_reset";
+			clocks = <&serdes_wiz0 TI_WIZ_PLL0_REFCLK>,
+				 <&serdes_wiz0 TI_WIZ_PHY_EN_REFCLK>;
+			clock-names = "refclk", "phy_en_refclk";
+			assigned-clocks = <&serdes_wiz0 TI_WIZ_PLL0_REFCLK>,
+					  <&serdes_wiz0 TI_WIZ_PLL1_REFCLK>,
+					  <&serdes_wiz0 TI_WIZ_REFCLK_DIG>;
+			assigned-clock-parents = <&k3_clks 404 6>,
+						 <&k3_clks 404 6>,
+						 <&k3_clks 404 6>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			#clock-cells = <1>;
+
+			status = "disabled";
+		};
+	};
+
+	serdes_wiz1: wiz@5070000 {
+		compatible = "ti,j784s4-wiz-10g";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		power-domains = <&k3_pds 405 TI_SCI_PD_EXCLUSIVE>;
+		clocks = <&k3_clks 405 2>, <&k3_clks 405 6>, <&serdes_refclk>, <&k3_clks 405 5>;
+		clock-names = "fck", "core_ref_clk", "ext_ref_clk", "core_ref1_clk";
+		assigned-clocks = <&k3_clks 405 6>;
+		assigned-clock-parents = <&k3_clks 405 10>;
+		num-lanes = <4>;
+		#reset-cells = <1>;
+		#clock-cells = <1>;
+		ranges = <0x05070000 0x00 0x05070000 0x10000>;
+
+		status = "disabled";
+
+		serdes1: serdes@5070000 {
+			compatible = "ti,j721e-serdes-10g";
+			reg = <0x05070000 0x010000>;
+			reg-names = "torrent_phy";
+			resets = <&serdes_wiz1 0>;
+			reset-names = "torrent_reset";
+			clocks = <&serdes_wiz1 TI_WIZ_PLL0_REFCLK>,
+				 <&serdes_wiz1 TI_WIZ_PHY_EN_REFCLK>;
+			clock-names = "refclk", "phy_en_refclk";
+			assigned-clocks = <&serdes_wiz1 TI_WIZ_PLL0_REFCLK>,
+					  <&serdes_wiz1 TI_WIZ_PLL1_REFCLK>,
+					  <&serdes_wiz1 TI_WIZ_REFCLK_DIG>;
+			assigned-clock-parents = <&k3_clks 405 6>,
+						 <&k3_clks 405 6>,
+						 <&k3_clks 405 6>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			#clock-cells = <1>;
+
+			status = "disabled";
+		};
+	};
+
+	serdes_wiz2: wiz@5020000 {
+		compatible = "ti,j784s4-wiz-10g";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		power-domains = <&k3_pds 406 TI_SCI_PD_EXCLUSIVE>;
+		clocks = <&k3_clks 406 2>, <&k3_clks 406 6>, <&serdes_refclk>, <&k3_clks 406 5>;
+		clock-names = "fck", "core_ref_clk", "ext_ref_clk", "core_ref1_clk";
+		assigned-clocks = <&k3_clks 406 6>;
+		assigned-clock-parents = <&k3_clks 406 10>;
+		num-lanes = <4>;
+		#reset-cells = <1>;
+		#clock-cells = <1>;
+		ranges = <0x05020000 0x00 0x05020000 0x10000>;
+
+		status = "disabled";
+
+		serdes2: serdes@5020000 {
+			compatible = "ti,j721e-serdes-10g";
+			reg = <0x05020000 0x010000>;
+			reg-names = "torrent_phy";
+			resets = <&serdes_wiz2 0>;
+			reset-names = "torrent_reset";
+			clocks = <&serdes_wiz2 TI_WIZ_PLL0_REFCLK>,
+				 <&serdes_wiz2 TI_WIZ_PHY_EN_REFCLK>;
+			clock-names = "refclk", "phy_en_refclk";
+			assigned-clocks = <&serdes_wiz2 TI_WIZ_PLL0_REFCLK>,
+					  <&serdes_wiz2 TI_WIZ_PLL1_REFCLK>,
+					  <&serdes_wiz2 TI_WIZ_REFCLK_DIG>;
+			assigned-clock-parents = <&k3_clks 406 6>,
+						 <&k3_clks 406 6>,
+						 <&k3_clks 406 6>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			#clock-cells = <1>;
+
+			status = "disabled";
+		};
+	};
+
+	serdes_wiz4: wiz@5050000 {
+		compatible = "ti,j784s4-wiz-10g";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		power-domains = <&k3_pds 407 TI_SCI_PD_EXCLUSIVE>;
+		clocks = <&k3_clks 407 2>, <&k3_clks 407 6>, <&serdes_refclk>, <&k3_clks 407 5>;
+		clock-names = "fck", "core_ref_clk", "ext_ref_clk", "core_ref1_clk";
+		assigned-clocks = <&k3_clks 407 6>;
+		assigned-clock-parents = <&k3_clks 407 10>;
+		num-lanes = <4>;
+		#reset-cells = <1>;
+		#clock-cells = <1>;
+		ranges = <0x05050000 0x00 0x05050000 0x10000>,
+			 <0xa030a00 0x00 0xa030a00 0x40>; /* DPTX PHY */
+
+		status = "disabled";
+
+		serdes4: serdes@5050000 {
+			/*
+			 * Note: we also map DPTX PHY registers as the Torrent
+			 * needs to manage those.
+			 */
+			compatible = "ti,j721e-serdes-10g";
+			reg = <0x05050000 0x010000>,
+			      <0x0a030a00 0x40>; /* DPTX PHY */
+			reg-names = "torrent_phy";
+			resets = <&serdes_wiz4 0>;
+			reset-names = "torrent_reset";
+			clocks = <&serdes_wiz4 TI_WIZ_PLL0_REFCLK>,
+				 <&serdes_wiz4 TI_WIZ_PHY_EN_REFCLK>;
+			clock-names = "refclk", "phy_en_refclk";
+			assigned-clocks = <&serdes_wiz4 TI_WIZ_PLL0_REFCLK>,
+					  <&serdes_wiz4 TI_WIZ_PLL1_REFCLK>,
+					  <&serdes_wiz4 TI_WIZ_REFCLK_DIG>;
+			assigned-clock-parents = <&k3_clks 407 6>,
+						 <&k3_clks 407 6>,
+						 <&k3_clks 407 6>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			#clock-cells = <1>;
+
+			status = "disabled";
+		};
+	};
+
 	main_navss: bus@30000000 {
 		compatible = "simple-bus";
 		#address-cells = <2>;