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

[Jean Pihet <jean.pihet@xxxxxxxxxxxxxx>]

Hi Tony,

On Tue, Dec 10, 2019 at 6:03 PM Tony Lindgren <tony@xxxxxxxxxxx> wrote:
>
> 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 :)

This patch works OK! The correct clock is in use by the driver. The
hwmod warning shows up at boot:
[    0.103567] omap_hwmod: qspi: no dt node
[    0.103599] ------------[ cut here ]------------
[    0.103639] WARNING: CPU: 0 PID: 1 at
arch/arm/mach-omap2/omap_hwmod.c:2414 _init.constprop.29+0x198/0x4a0
[    0.103654] omap_hwmod: qspi: doesn't have mpu register target base

Glad to help to get to the final solution, please let me know how I
can help on that.

Regards,
Jean

>
> 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