| Message ID | 20230710101705.154119-1-j-choudhary@ti.com |
|---|---|
| Headers | show |
| Series | Add peripherals for J784S4 | expand |
On 11/07/2023 17:31, Nishanth Menon wrote: > On 12:01-20230711, Jayesh Choudhary wrote: >> >> >> On 10/07/23 17:13, Krzysztof Kozlowski wrote: >>> On 10/07/2023 12:17, Jayesh Choudhary wrote: >>>> From: Siddharth Vadapalli <s-vadapalli@ti.com> >>>> >>>> The system controller node manages the CTRL_MMR0 region. >>>> Add serdes_ln_ctrl node which is used for controlling the SERDES lane mux. >>>> >>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>> [j-choudhary@ti.com: Add reg property to fix dtc warning] >>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com> >>>> --- >>>> arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi | 23 ++++++++++++++++++++++ >>>> 1 file changed, 23 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi >>>> index 2ea0adae6832..68cc2fa053e7 100644 >>>> --- a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi >>>> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi >>>> @@ -5,6 +5,9 @@ >>>> * Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/ >>>> */ >>>> +#include <dt-bindings/mux/mux.h> >>>> +#include <dt-bindings/mux/ti-serdes.h> >>> >>> Why? What do you use from that binding? >>> >> >> Missed idle-state in the mux-controller node here for default values. >> I will wait for more feedback and then re-spin the series. > > btw, I am wondering if ti-serdes.h should even exist in dt-bindings - > are any of the macros used in the driver? or should this follow the > pinctrl style macros that could happily reside in arch/arm64/boot/dts/ti > ? I don't see any usage in drivers, which is a clear indication that it might not be suitable for bindings. What are these values? Look like some register values, which there is little sense in making a binding. Best regards, Krzysztof
On 12/07/2023 08:44, Krzysztof Kozlowski wrote: > On 11/07/2023 17:31, Nishanth Menon wrote: >> On 12:01-20230711, Jayesh Choudhary wrote: >>> >>> >>> On 10/07/23 17:13, Krzysztof Kozlowski wrote: >>>> On 10/07/2023 12:17, Jayesh Choudhary wrote: >>>>> From: Siddharth Vadapalli <s-vadapalli@ti.com> >>>>> >>>>> The system controller node manages the CTRL_MMR0 region. >>>>> Add serdes_ln_ctrl node which is used for controlling the SERDES lane mux. >>>>> >>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>>> [j-choudhary@ti.com: Add reg property to fix dtc warning] >>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com> >>>>> --- >>>>> arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi | 23 ++++++++++++++++++++++ >>>>> 1 file changed, 23 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi >>>>> index 2ea0adae6832..68cc2fa053e7 100644 >>>>> --- a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi >>>>> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi >>>>> @@ -5,6 +5,9 @@ >>>>> * Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/ >>>>> */ >>>>> +#include <dt-bindings/mux/mux.h> >>>>> +#include <dt-bindings/mux/ti-serdes.h> >>>> >>>> Why? What do you use from that binding? >>>> >>> >>> Missed idle-state in the mux-controller node here for default values. >>> I will wait for more feedback and then re-spin the series. >> >> btw, I am wondering if ti-serdes.h should even exist in dt-bindings - >> are any of the macros used in the driver? or should this follow the >> pinctrl style macros that could happily reside in arch/arm64/boot/dts/ti >> ? > > I don't see any usage in drivers, which is a clear indication that it > might not be suitable for bindings. What are these values? Look like > some register values, which there is little sense in making a binding. > > Best regards, > Krzysztof > > You are right. They are constants not used in the driver directly. mmio-mux driver uses it to set the idle state of the mux via the 'idle-states' property. I agree with Nishanth that they should be moved to arch/arm64/boot/dts/ti
On 12/07/23 16:51, Roger Quadros wrote: > > > On 12/07/2023 08:44, Krzysztof Kozlowski wrote: >> On 11/07/2023 17:31, Nishanth Menon wrote: >>> On 12:01-20230711, Jayesh Choudhary wrote: >>>> >>>> >>>> On 10/07/23 17:13, Krzysztof Kozlowski wrote: >>>>> On 10/07/2023 12:17, Jayesh Choudhary wrote: >>>>>> From: Siddharth Vadapalli <s-vadapalli@ti.com> >>>>>> >>>>>> The system controller node manages the CTRL_MMR0 region. >>>>>> Add serdes_ln_ctrl node which is used for controlling the SERDES lane mux. >>>>>> >>>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>>>> [j-choudhary@ti.com: Add reg property to fix dtc warning] >>>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com> >>>>>> --- >>>>>> arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi | 23 ++++++++++++++++++++++ >>>>>> 1 file changed, 23 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi >>>>>> index 2ea0adae6832..68cc2fa053e7 100644 >>>>>> --- a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi >>>>>> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi >>>>>> @@ -5,6 +5,9 @@ >>>>>> * Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/ >>>>>> */ >>>>>> +#include <dt-bindings/mux/mux.h> >>>>>> +#include <dt-bindings/mux/ti-serdes.h> >>>>> >>>>> Why? What do you use from that binding? >>>>> >>>> >>>> Missed idle-state in the mux-controller node here for default values. >>>> I will wait for more feedback and then re-spin the series. >>> >>> btw, I am wondering if ti-serdes.h should even exist in dt-bindings - >>> are any of the macros used in the driver? or should this follow the >>> pinctrl style macros that could happily reside in arch/arm64/boot/dts/ti >>> ? >> >> I don't see any usage in drivers, which is a clear indication that it >> might not be suitable for bindings. What are these values? Look like >> some register values, which there is little sense in making a binding. >> >> Best regards, >> Krzysztof >> >> > > You are right. They are constants not used in the driver directly. > mmio-mux driver uses it to set the idle state of the mux via the > 'idle-states' property. > > I agree with Nishanth that they should be moved to arch/arm64/boot/dts/ti > Then I will do the cleanup for all platforms and then post the dependent series before spinning v6. Thanks and Warm regards, -Jayesh
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 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.
This series adds support for: - SERDES, WIZ DT nodes, Serdes lane control mux - MAIN CPSW2G nodes - DSS and DisplayPort-0 nodes Changelog v4->v5: - rebased the patches on linux-next tip. Changelog v3->v4: - add reg property to serdes_ln_ctrl and fix the node name again to get rid of dtbs_check error. - reorder reg, reg-names and ranges property for main_cpsw1. - correct the order for clocks in serdes_wiz nodes to fix dtbs_check warnings. - fix indentation in reg, reg-names and clock property for dss node. - add comments for the reg type in dss registers. Changelog v3->v2: - fix dtc warnings for 'scm_conf' and 'serdes_ln_ctrl' nodes (Checked all the changes of the series with W=12 option during build) - added clock-frequency for serdes_refclk along with other EVM changes This refclk is being used by all the instances of serdes_wiz which are disabled by default. So configuring refclk when the serdes nodes are used for the first time is okay. Changelog v1->v2: - Moved J784S4 EVM changes together to the last patch (Suggested by Andrew) v4 patch link: <https://lore.kernel.org/all/20230425131607.290707-1-j-choudhary@ti.com/> Rahul T R (2): arm64: dts: ti: k3-j784s4-main: Add DSS and DP-bridge node arm64: dts: ti: k3-j784s4-evm: Enable DisplayPort-0 Siddharth Vadapalli (3): arm64: dts: ti: k3-j784s4-main: Add system controller and SERDES lane mux arm64: dts: ti: k3-j784s4: Add Main CPSW2G node arm64: dts: ti: k3-j784s4: Add WIZ and SERDES PHY nodes arch/arm64/boot/dts/ti/k3-j784s4-evm.dts | 164 ++++++++++ arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi | 335 +++++++++++++++++++++ 2 files changed, 499 insertions(+)