Re: [PATCH 1/3] TI QSPI: Fix fclk frequency

[Tony Lindgren <tony@xxxxxxxxxxx>]

Hi,

* Jean Pihet <jean.pihet@xxxxxxxxxxxxxx> [191210 16:24]:
> On Fri, Dec 6, 2019 at 6:57 PM Tony Lindgren <tony@xxxxxxxxxxx> wrote:
> > Care to test with the patch below (without your changes) to see if
> > something else is still needed?
> 
> With the patch applied fck still is the ick, not the L4 clock as required.

Hmm OK so I think we need fck at both the module level and qspi level.

> Could both ick and fck be defined in the DT files, like here below?
>    clocks = <&l3s_clkctrl AM4_L3S_QSPI_CLKCTRL 0>,
>                  <&l4per2_clkctrl AM4_L4PER2_QSPI_CLKCTRL 0>;
>    clock-names = "ick", "fck";
> The issue is that there is no l4_per for AM4.

Yes you can configure both fck and ick there, and also additional
clocks. But the clkctrl clock is the fck clock gate for this module,
and it's source can be the same as the interface clock for some
modules.

When I sent the experimental patch I confirmed that just the fck
as <&l3s_clkctrl AM4_L3S_QSPI_CLKCTRL 0>, ti-sysc.c driver can
read the qspi module revision register just fine. So that means
that it's enough for the module, and the spi_clk is another
clock specific to the child qspi IP in the module.

So based on that, I think we should set up the clocks in the
following way for the module and it's qspi child:

target-module@47900000 {
	...
	clocks = <&l3s_clkctrl AM4_L3S_QSPI_CLKCTRL 0>;
	clock-names = "fck";
	...

	qspi: spi@0 {
		...
		clocks = <&dpll_per_m2_div4_ck>;
		clock-names = "fck";
		...
	};
};

That way the qspi driver can set the divider on it's fck based
on "spi-max-frequency" dts property.

> Looking at the DRA7 DT files there is an fck defined (in dra7.dtsi):
>    clocks = <&l4per2_clkctrl DRA7_L4PER2_QSPI_CLKCTRL 25>;
>    clock-names = "fck";

Yeah so that's <&l3s_clkctrl AM4_L3S_QSPI_CLKCTRL 0> for
am437x.

> What is best to do from here?

Well can test again with the patch below to see if that is
enough to make it work :)

Regards,

Tony

8< -------------------
diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
--- a/arch/arm/boot/dts/am4372.dtsi
+++ b/arch/arm/boot/dts/am4372.dtsi
@@ -302,17 +302,35 @@ gpmc: gpmc@50000000 {
 			status = "disabled";
 		};
 
-		qspi: spi@47900000 {
-			compatible = "ti,am4372-qspi";
-			reg = <0x47900000 0x100>,
-			      <0x30000000 0x4000000>;
-			reg-names = "qspi_base", "qspi_mmap";
+		target-module@47900000 {
+			compatible = "ti,sysc-omap4", "ti,sysc";
+			//ti,hwmods = "qspi";
+			reg = <0x47900000 0x4>,
+			      <0x47900010 0x4>;
+			reg-names = "rev", "sysc";
+			ti,sysc-sidle = <SYSC_IDLE_FORCE>,
+					<SYSC_IDLE_NO>,
+					<SYSC_IDLE_SMART>,
+					<SYSC_IDLE_SMART_WKUP>;
+			clocks = <&l3s_clkctrl AM4_L3S_QSPI_CLKCTRL 0>;
+			clock-names = "fck";
 			#address-cells = <1>;
-			#size-cells = <0>;
-			ti,hwmods = "qspi";
-			interrupts = <0 138 0x4>;
-			num-cs = <4>;
-			status = "disabled";
+			#size-cells = <1>;
+			ranges = <0x0 0x47900000 0x1000>,
+				 <0x30000000 0x30000000 0x4000000>;
+
+			qspi: spi@0 {
+				compatible = "ti,am4372-qspi";
+				reg = <0 0x100>,
+				      <0x30000000 0x4000000>;
+				reg-names = "qspi_base", "qspi_mmap";
+				clocks = <&dpll_per_m2_div4_ck>;
+				clock-names = "fck";
+				#address-cells = <1>;
+				#size-cells = <0>;
+				interrupts = <0 138 0x4>;
+				num-cs = <4>;
+			};
 		};
 
 		dss: dss@4832a000 {
-- 
2.24.0