| Message ID | 20230710101705.154119-4-j-choudhary@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add peripherals for J784S4 | expand |
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
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>
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.
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
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.
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
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
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>;