| Message ID | 20250211151725.4133582-3-matthew.gerlach@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | Add PCIe Root Port support for Agilex family of chips | expand |
On 11/02/2025 16:17, Matthew Gerlach wrote: > Add required clock-frequency property to fixed-clock nodes > to fix schema check warnings. > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > --- > v6: > - New patch to series. > --- > arch/arm64/boot/dts/intel/socfpga_agilex.dtsi | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi > index 1235ba5a9865..42cb24cfa6da 100644 > --- a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi > +++ b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi > @@ -114,21 +114,25 @@ clocks { > cb_intosc_hs_div2_clk: cb-intosc-hs-div2-clk { > #clock-cells = <0>; > compatible = "fixed-clock"; > + clock-frequency = <0>; That's not a correct frequency. You silence some error by introducing incorrect properties. That's wrong. Don't fix the warnings just to silence them, while keeping actual errors still in the code. Best regards, Krzysztof
On Wed, 12 Feb 2025, Krzysztof Kozlowski wrote: > On 11/02/2025 16:17, Matthew Gerlach wrote: >> Add required clock-frequency property to fixed-clock nodes >> to fix schema check warnings. >> >> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> --- >> v6: >> - New patch to series. >> --- >> arch/arm64/boot/dts/intel/socfpga_agilex.dtsi | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi >> index 1235ba5a9865..42cb24cfa6da 100644 >> --- a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi >> +++ b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi >> @@ -114,21 +114,25 @@ clocks { >> cb_intosc_hs_div2_clk: cb-intosc-hs-div2-clk { >> #clock-cells = <0>; >> compatible = "fixed-clock"; >> + clock-frequency = <0>; > > That's not a correct frequency. You silence some error by introducing > incorrect properties. That's wrong. A clock-frequency of 0 seems valid for a clock that is disabled or not used on a particular board. I chose this approach because it already has widespread usage in the kernel: grep 'clock-frequency = <0>' arch/arm64/boot/dts/*/*.dtsi | wc -l 198 > > Don't fix the warnings just to silence them, while keeping actual errors > still in the code. I actually want to fix the existing warnings, but it seems appropriate to only address the existing warnings that are related to this patch set of adding PCIe Root Port support to the Agilex family of chips. This patch set requires touching the file, socfpga_agilex.dtsi; so I fixed the warnings I thought were in this file. I believe the other warnings need to be fixed by converting text binding descriptions to yaml or by touching files unrelated to this patch set. Setting the value of the status property to "disabled" also silences the particular fixed-clock, but I didn't see any other usage by a fixed-clock. What do suggest is the best way to handle this warning? > > Best regards, > Krzysztof > Thanks for the feedback, Matthew Gerlach
On 13/02/2025 18:37, matthew.gerlach@linux.intel.com wrote: > > > On Wed, 12 Feb 2025, Krzysztof Kozlowski wrote: > >> On 11/02/2025 16:17, Matthew Gerlach wrote: >>> Add required clock-frequency property to fixed-clock nodes >>> to fix schema check warnings. >>> >>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >>> --- >>> v6: >>> - New patch to series. >>> --- >>> arch/arm64/boot/dts/intel/socfpga_agilex.dtsi | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi >>> index 1235ba5a9865..42cb24cfa6da 100644 >>> --- a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi >>> +++ b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi >>> @@ -114,21 +114,25 @@ clocks { >>> cb_intosc_hs_div2_clk: cb-intosc-hs-div2-clk { >>> #clock-cells = <0>; >>> compatible = "fixed-clock"; >>> + clock-frequency = <0>; >> >> That's not a correct frequency. You silence some error by introducing >> incorrect properties. That's wrong. > > A clock-frequency of 0 seems valid for a clock that is disabled or not > used on a particular board. I chose this approach because it already has > widespread usage in the kernel: > > grep 'clock-frequency = <0>' arch/arm64/boot/dts/*/*.dtsi | wc -l > 198 If the clock is not there, it should be removed or disabled. Otherwise what is the point of setting here clock frequency=0? To silence some warning. Why is there warning in the first place? > >> >> Don't fix the warnings just to silence them, while keeping actual errors >> still in the code. > > I actually want to fix the existing warnings, but it seems appropriate to > only address the existing warnings that are related to this patch set of > adding PCIe Root Port support to the Agilex family of chips. This patch > set requires touching the file, socfpga_agilex.dtsi; so I fixed the > warnings I thought were in this file. I believe the other warnings need to > be fixed by converting text binding descriptions to yaml or by touching > files unrelated to this patch set. > > Setting the value of the status property to "disabled" also silences the > particular fixed-clock, but I didn't see any other usage by a fixed-clock. > What do suggest is the best way to handle this warning? DTSI and DTS represent actual hardware, so common DTSI should not have clocks which do not exist. Such clocks should be in DTS or some other common DTSI file, depending on how the hardware is organized. Instead of fixing the warning, look at the cause - what is wrong in DTSI in this hardware description. Best regards, Krzysztof
diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi index 1235ba5a9865..42cb24cfa6da 100644 --- a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi +++ b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi @@ -114,21 +114,25 @@ clocks { cb_intosc_hs_div2_clk: cb-intosc-hs-div2-clk { #clock-cells = <0>; compatible = "fixed-clock"; + clock-frequency = <0>; }; cb_intosc_ls_clk: cb-intosc-ls-clk { #clock-cells = <0>; compatible = "fixed-clock"; + clock-frequency = <0>; }; f2s_free_clk: f2s-free-clk { #clock-cells = <0>; compatible = "fixed-clock"; + clock-frequency = <0>; }; osc1: osc1 { #clock-cells = <0>; compatible = "fixed-clock"; + clock-frequency = <0>; }; qspi_clk: qspi-clk {
Add required clock-frequency property to fixed-clock nodes to fix schema check warnings. Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> --- v6: - New patch to series. --- arch/arm64/boot/dts/intel/socfpga_agilex.dtsi | 4 ++++ 1 file changed, 4 insertions(+)