diff mbox

clk: mxs: ensure that i.MX28's ref_io clks are not operated too fast

Message ID 20170503185625.10297-1-u.kleine-koenig@pengutronix.de (mailing list archive)
State Deferred
Headers show

Commit Message

Uwe Kleine-König May 3, 2017, 6:56 p.m. UTC
Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and
64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for
SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized
to 288 MHz because the initial frequency "seems too high to be ssp clock
source directly". However this isn't enough to ensure that the frequency
isn't increased later again. In fact this happens on my machine as the
mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being
ssp2 which is resolved to

	ref_io1.rate = 320 MHz
	ssp2_sel.parent = ref_io1
	ssp2_div.divider = 2

. The observed effect is that reading MISO reliably fails: Instead of
the least significant bit the second least significant bit is reported
twice. This is probably related to the reports

	https://community.nxp.com/thread/290209
	https://community.nxp.com/thread/310434
	https://community.nxp.com/thread/385546

So additionally to initializing ref_io0 and ref_io1 to 288 MHz ensure
that their frequency is never set to something bigger later on. This is
done by adding a parameter min_frac to clk-ref and use that instead of
the hard coded 18 to limit the valid values for FRAC. For all ref clocks
but ref_io0 and ref_io1 18 is used and so there is no functional change
for those.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

I'd expect this to go in as a fix as it repairs reading from an EEPROM which
should be a fairly common task.

It would be nice to get confirmation from NXP that this is a right fix and
that spi transfers are supposed to be reliable with ref_ioX <= 288 MHz.

Best regards
Uwe

 drivers/clk/mxs/clk-imx23.c |  8 ++++----
 drivers/clk/mxs/clk-imx28.c | 14 +++++++-------
 drivers/clk/mxs/clk-ref.c   | 18 ++++++++++++------
 drivers/clk/mxs/clk.h       |  2 +-
 4 files changed, 24 insertions(+), 18 deletions(-)

Comments

Fabio Estevam May 3, 2017, 7:53 p.m. UTC | #1
On Wed, May 3, 2017 at 3:56 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and
> 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for
> SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized
> to 288 MHz because the initial frequency "seems too high to be ssp clock
> source directly". However this isn't enough to ensure that the frequency
> isn't increased later again. In fact this happens on my machine as the
> mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being
> ssp2 which is resolved to
>
>         ref_io1.rate = 320 MHz
>         ssp2_sel.parent = ref_io1
>         ssp2_div.divider = 2
>
> . The observed effect is that reading MISO reliably fails: Instead of
> the least significant bit the second least significant bit is reported
> twice. This is probably related to the reports
>
>         https://community.nxp.com/thread/290209
>         https://community.nxp.com/thread/310434

Adding Stefan on Cc in he could help testing this patch, as he was the
one that created this thread.


>         https://community.nxp.com/thread/385546
>
> So additionally to initializing ref_io0 and ref_io1 to 288 MHz ensure
> that their frequency is never set to something bigger later on. This is
> done by adding a parameter min_frac to clk-ref and use that instead of
> the hard coded 18 to limit the valid values for FRAC. For all ref clocks
> but ref_io0 and ref_io1 18 is used and so there is no functional change
> for those.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
>
> I'd expect this to go in as a fix as it repairs reading from an EEPROM which
> should be a fairly common task.
>
> It would be nice to get confirmation from NXP that this is a right fix and
> that spi transfers are supposed to be reliable with ref_ioX <= 288 MHz.
>
> Best regards
> Uwe
>
>  drivers/clk/mxs/clk-imx23.c |  8 ++++----
>  drivers/clk/mxs/clk-imx28.c | 14 +++++++-------
>  drivers/clk/mxs/clk-ref.c   | 18 ++++++++++++------
>  drivers/clk/mxs/clk.h       |  2 +-
>  4 files changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/clk/mxs/clk-imx23.c b/drivers/clk/mxs/clk-imx23.c
> index f01876af6bb8..b7d3c44bbcbb 100644
> --- a/drivers/clk/mxs/clk-imx23.c
> +++ b/drivers/clk/mxs/clk-imx23.c
> @@ -117,10 +117,10 @@ static void __init mx23_clocks_init(struct device_node *np)
>
>         clks[ref_xtal] = mxs_clk_fixed("ref_xtal", 24000000);
>         clks[pll] = mxs_clk_pll("pll", "ref_xtal", PLLCTRL0, 16, 480000000);
> -       clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll", FRAC, 0);
> -       clks[ref_emi] = mxs_clk_ref("ref_emi", "pll", FRAC, 1);
> -       clks[ref_pix] = mxs_clk_ref("ref_pix", "pll", FRAC, 2);
> -       clks[ref_io] = mxs_clk_ref("ref_io", "pll", FRAC, 3);
> +       clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll", FRAC, 0, 18);
> +       clks[ref_emi] = mxs_clk_ref("ref_emi", "pll", FRAC, 1, 18);
> +       clks[ref_pix] = mxs_clk_ref("ref_pix", "pll", FRAC, 2, 18);
> +       clks[ref_io] = mxs_clk_ref("ref_io", "pll", FRAC, 3, 18);
>         clks[saif_sel] = mxs_clk_mux("saif_sel", CLKSEQ, 0, 1, sel_pll, ARRAY_SIZE(sel_pll));
>         clks[lcdif_sel] = mxs_clk_mux("lcdif_sel", CLKSEQ, 1, 1, sel_pix, ARRAY_SIZE(sel_pix));
>         clks[gpmi_sel] = mxs_clk_mux("gpmi_sel", CLKSEQ, 4, 1, sel_io, ARRAY_SIZE(sel_io));
> diff --git a/drivers/clk/mxs/clk-imx28.c b/drivers/clk/mxs/clk-imx28.c
> index 6b572b759f9a..09e324cbd32f 100644
> --- a/drivers/clk/mxs/clk-imx28.c
> +++ b/drivers/clk/mxs/clk-imx28.c
> @@ -174,13 +174,13 @@ static void __init mx28_clocks_init(struct device_node *np)
>         clks[pll0] = mxs_clk_pll("pll0", "ref_xtal", PLL0CTRL0, 17, 480000000);
>         clks[pll1] = mxs_clk_pll("pll1", "ref_xtal", PLL1CTRL0, 17, 480000000);
>         clks[pll2] = mxs_clk_pll("pll2", "ref_xtal", PLL2CTRL0, 23, 50000000);
> -       clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll0", FRAC0, 0);
> -       clks[ref_emi] = mxs_clk_ref("ref_emi", "pll0", FRAC0, 1);
> -       clks[ref_io1] = mxs_clk_ref("ref_io1", "pll0", FRAC0, 2);
> -       clks[ref_io0] = mxs_clk_ref("ref_io0", "pll0", FRAC0, 3);
> -       clks[ref_pix] = mxs_clk_ref("ref_pix", "pll0", FRAC1, 0);
> -       clks[ref_hsadc] = mxs_clk_ref("ref_hsadc", "pll0", FRAC1, 1);
> -       clks[ref_gpmi] = mxs_clk_ref("ref_gpmi", "pll0", FRAC1, 2);
> +       clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll0", FRAC0, 0, 18);
> +       clks[ref_emi] = mxs_clk_ref("ref_emi", "pll0", FRAC0, 1, 18);
> +       clks[ref_io1] = mxs_clk_ref("ref_io1", "pll0", FRAC0, 2, 30);
> +       clks[ref_io0] = mxs_clk_ref("ref_io0", "pll0", FRAC0, 3, 30);
> +       clks[ref_pix] = mxs_clk_ref("ref_pix", "pll0", FRAC1, 0, 18);
> +       clks[ref_hsadc] = mxs_clk_ref("ref_hsadc", "pll0", FRAC1, 1, 18);
> +       clks[ref_gpmi] = mxs_clk_ref("ref_gpmi", "pll0", FRAC1, 2, 18);
>         clks[saif0_sel] = mxs_clk_mux("saif0_sel", CLKSEQ, 0, 1, sel_pll0, ARRAY_SIZE(sel_pll0));
>         clks[saif1_sel] = mxs_clk_mux("saif1_sel", CLKSEQ, 1, 1, sel_pll0, ARRAY_SIZE(sel_pll0));
>         clks[gpmi_sel] = mxs_clk_mux("gpmi_sel", CLKSEQ, 2, 1, sel_gpmi, ARRAY_SIZE(sel_gpmi));
> diff --git a/drivers/clk/mxs/clk-ref.c b/drivers/clk/mxs/clk-ref.c
> index 495f99b7965e..9385563b3ea8 100644
> --- a/drivers/clk/mxs/clk-ref.c
> +++ b/drivers/clk/mxs/clk-ref.c
> @@ -23,13 +23,17 @@
>   *
>   * The mxs reference clock sources from pll.  Every 4 reference clocks share
>   * one register space, and @idx is used to identify them.  Each reference
> - * clock has a gate control and a fractional * divider.  The rate is calculated
> + * clock has a gate control and a fractional divider.  The rate is calculated
>   * as pll rate  * (18 / FRAC), where FRAC = 18 ~ 35.
> + *
> + * As some clocks are not reliable at high speed (imx28 io0 and io1), the clock
> + * can be limited to not use FRAC < min_frac.
>   */
>  struct clk_ref {
>         struct clk_hw hw;
>         void __iomem *reg;
>         u8 idx;
> +       u8 min_frac;
>  };
>
>  #define to_clk_ref(_hw) container_of(_hw, struct clk_ref, hw)
> @@ -66,6 +70,7 @@ static unsigned long clk_ref_recalc_rate(struct clk_hw *hw,
>  static long clk_ref_round_rate(struct clk_hw *hw, unsigned long rate,
>                                unsigned long *prate)
>  {
> +       struct clk_ref *ref = to_clk_ref(hw);
>         unsigned long parent_rate = *prate;
>         u64 tmp = parent_rate;
>         u8 frac;
> @@ -74,8 +79,8 @@ static long clk_ref_round_rate(struct clk_hw *hw, unsigned long rate,
>         do_div(tmp, rate);
>         frac = tmp;
>
> -       if (frac < 18)
> -               frac = 18;
> +       if (frac < ref->min_frac)
> +               frac = ref->min_frac;
>         else if (frac > 35)
>                 frac = 35;
>
> @@ -99,8 +104,8 @@ static int clk_ref_set_rate(struct clk_hw *hw, unsigned long rate,
>         do_div(tmp, rate);
>         frac = tmp;
>
> -       if (frac < 18)
> -               frac = 18;
> +       if (frac < ref->min_frac)
> +               frac = ref->min_frac;
>         else if (frac > 35)
>                 frac = 35;
>
> @@ -125,7 +130,7 @@ static const struct clk_ops clk_ref_ops = {
>  };
>
>  struct clk *mxs_clk_ref(const char *name, const char *parent_name,
> -                       void __iomem *reg, u8 idx)
> +                       void __iomem *reg, u8 idx, u8 min_frac)
>  {
>         struct clk_ref *ref;
>         struct clk *clk;
> @@ -143,6 +148,7 @@ struct clk *mxs_clk_ref(const char *name, const char *parent_name,
>
>         ref->reg = reg;
>         ref->idx = idx;
> +       ref->min_frac = min_frac;
>         ref->hw.init = &init;
>
>         clk = clk_register(NULL, &ref->hw);
> diff --git a/drivers/clk/mxs/clk.h b/drivers/clk/mxs/clk.h
> index 5a264a486ad9..fb63462a78cd 100644
> --- a/drivers/clk/mxs/clk.h
> +++ b/drivers/clk/mxs/clk.h
> @@ -28,7 +28,7 @@ struct clk *mxs_clk_pll(const char *name, const char *parent_name,
>                         void __iomem *base, u8 power, unsigned long rate);
>
>  struct clk *mxs_clk_ref(const char *name, const char *parent_name,
> -                       void __iomem *reg, u8 idx);
> +                       void __iomem *reg, u8 idx, u8 min_frac);
>
>  struct clk *mxs_clk_div(const char *name, const char *parent_name,
>                         void __iomem *reg, u8 shift, u8 width, u8 busy);
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefan Wahren May 4, 2017, 12:25 p.m. UTC | #2
Hi,

Am 03.05.2017 um 21:53 schrieb Fabio Estevam:
> On Wed, May 3, 2017 at 3:56 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
>> Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and
>> 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for
>> SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized
>> to 288 MHz because the initial frequency "seems too high to be ssp clock
>> source directly". However this isn't enough to ensure that the frequency
>> isn't increased later again. In fact this happens on my machine as the
>> mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being
>> ssp2 which is resolved to
>>
>>         ref_io1.rate = 320 MHz
>>         ssp2_sel.parent = ref_io1
>>         ssp2_div.divider = 2
>>
>> . The observed effect is that reading MISO reliably fails: Instead of
>> the least significant bit the second least significant bit is reported
>> twice. This is probably related to the reports
>>
>>         https://community.nxp.com/thread/290209
>>         https://community.nxp.com/thread/310434
> Adding Stefan on Cc in he could help testing this patch, as he was the
> one that created this thread.

thanks for forwarding. I tested the patch with Linux 4.11 it on our
Duckbill (i.MX28), but it didn't fix the SPI bit errors with the
QCA7000. Currently the only way to get the QCA7000 working is to switch
the clock parent of ssp2 to ref_xtal (24 MHz).

Is there someone at NXP/Freescale who could explain this behavior?
Are there any relevant register values which could be helpful?

I guess it's a special ratio between ref_io and ssp_div which causes the
issue.

Here are the clk_summary for Linux 4.11 on Duckbill with QCA7000 on ssp
2 for all 3 cases:

1) unpatched (= always bit errors on ssp2)

   clock                         enable_cnt  prepare_cnt        rate  
accuracy   phase
----------------------------------------------------------------------------------------
 ref_xtal                                 5            5   
24000000          0 0
    can1                                  0            0   
24000000          0 0
    can0                                  0            0   
24000000          0 0
    uart                                  2            3   
24000000          0 0
    pwm                                   0            0   
24000000          0 0
    rtc                                   0            0      
31250          0 0
    clk32k_div                            1            1      
32000          0 0
       clk32k                             1            1      
32000          0 0
          lradc                           1            1       
2000          0 0
    emi_xtal                              0            0   
24000000          0 0
    xbus                                  3            3   
24000000          0 0
    cpu_xtal                              0            0   
24000000          0 0
    ptp_sel                               0            0   
24000000          0 0
       ptp                                0            0   
24000000          0 0
    lcdif_sel                             0            0   
24000000          0 0
       lcdif_div                          0            0   
24000000          0 0
          lcdif                           0            0   
24000000          0 0
    etm_sel                               0            0   
24000000          0 0
       etm_div                            0            0   
24000000          0 0
          etm                             0            0   
24000000          0 0
    gpmi_sel                              0            0   
24000000          0 0
       gpmi_div                           0            0   
24000000          0 0
          gpmi                            0            0   
24000000          0 0
    pll2                                  1            1   
50000000          0 0
       enet_out                           1            1   
50000000          0 0
    pll1                                  0            0  
480000000          0 0
       usb1_phy                           0            0  
480000000          0 0
          usb1                            0            0  
480000000          0 0
    pll0                                  5            5  
480000000          0 0
       usb0_phy                           2            2  
480000000          0 0
          usb0                            1            1  
480000000          0 0
       spdif_div                          0            0  
120000000          0 0
          spdif                           0            0  
120000000          0 0
       saif1_sel                          0            0  
480000000          0 0
          saif1_div                       0            0       
7324          0 0
             saif1                        0            0       
7324          0 0
       saif0_sel                          0            0  
480000000          0 0
          saif0_div                       0            0       
7324          0 0
             saif0                        0            0       
7324          0 0
       ref_gpmi                           0            0  
480000000          0 0
       ref_hsadc                          0            0  
480000000          0 0
       ref_pix                            0            0  
480000000          0 0
       ref_io0                            1            1  
288000000          0 0
          ssp1_sel                        0            0  
288000000          0 0
             ssp1_div                     0            0  
288000000          0 0
                ssp1                      0            0  
288000000          0 0
          ssp0_sel                        1            1  
288000000          0 0
             ssp0_div                     1            1   
57600000          0 0
                ssp0                      1            1   
57600000          0 0
       ref_io1                            1            1  
320000000          0 0
          ssp3_sel                        0            0  
320000000          0 0
             ssp3_div                     0            0  
320000000          0 0
                ssp3                      0            0  
320000000          0 0
          ssp2_sel                        1            1  
320000000          0 0
             ssp2_div                     1            1  
160000000          0 0
                ssp2                      1            1  
160000000          0 0
       ref_emi                            1            1  
411428571          0 0
          emi_pll                         1            1  
205714286          0 0
             emi_sel                      1            1  
205714286          0 0
                emi                       1            1  
205714286          0 0
       ref_cpu                            1            1  
454736842          0 0
          cpu_pll                         1            1  
454736842          0 0
             cpu                          2            2  
454736842          0 0
                hbus                      4            4  
151578948          0 0
                   fec                    2            2  
151578948          0 0

2) ref_xtal as clock parent of ssp2 (= no bit errors on ssp2)

   clock                         enable_cnt  prepare_cnt        rate  
accuracy   phase
----------------------------------------------------------------------------------------
 ref_xtal                                 6            6   
24000000          0 0
    can1                                  0            0   
24000000          0 0
    can0                                  0            0   
24000000          0 0
    uart                                  2            3   
24000000          0 0
    pwm                                   0            0   
24000000          0 0
    rtc                                   0            0      
31250          0 0
    clk32k_div                            1            1      
32000          0 0
       clk32k                             1            1      
32000          0 0
          lradc                           1            1       
2000          0 0
    emi_xtal                              0            0   
24000000          0 0
    xbus                                  3            3   
24000000          0 0
    cpu_xtal                              0            0   
24000000          0 0
    ptp_sel                               0            0   
24000000          0 0
       ptp                                0            0   
24000000          0 0
    lcdif_sel                             0            0   
24000000          0 0
       lcdif_div                          0            0   
24000000          0 0
          lcdif                           0            0   
24000000          0 0
    etm_sel                               0            0   
24000000          0 0
       etm_div                            0            0   
24000000          0 0
          etm                             0            0   
24000000          0 0
    ssp2_sel                              1            1   
24000000          0 0
       ssp2_div                           1            1   
24000000          0 0
          ssp2                            1            1   
24000000          0 0
    gpmi_sel                              0            0   
24000000          0 0
       gpmi_div                           0            0   
24000000          0 0
          gpmi                            0            0   
24000000          0 0
    pll2                                  1            1   
50000000          0 0
       enet_out                           1            1   
50000000          0 0
    pll1                                  0            0  
480000000          0 0
       usb1_phy                           0            0  
480000000          0 0
          usb1                            0            0  
480000000          0 0
    pll0                                  4            4  
480000000          0 0
       usb0_phy                           2            2  
480000000          0 0
          usb0                            1            1  
480000000          0 0
       spdif_div                          0            0  
120000000          0 0
          spdif                           0            0  
120000000          0 0
       saif1_sel                          0            0  
480000000          0 0
          saif1_div                       0            0       
7324          0 0
             saif1                        0            0       
7324          0 0
       saif0_sel                          0            0  
480000000          0 0
          saif0_div                       0            0       
7324          0 0
             saif0                        0            0       
7324          0 0
       ref_gpmi                           0            0  
480000000          0 0
       ref_hsadc                          0            0  
480000000          0 0
       ref_pix                            0            0  
480000000          0 0
       ref_io0                            1            1  
288000000          0 0
          ssp1_sel                        0            0  
288000000          0 0
             ssp1_div                     0            0  
288000000          0 0
                ssp1                      0            0  
288000000          0 0
          ssp0_sel                        1            1  
288000000          0 0
             ssp0_div                     1            1   
57600000          0 0
                ssp0                      1            1   
57600000          0 0
       ref_io1                            0            0  
288000000          0 0
          ssp3_sel                        0            0  
288000000          0 0
             ssp3_div                     0            0  
288000000          0 0
                ssp3                      0            0  
288000000          0 0
       ref_emi                            1            1  
411428571          0 0
          emi_pll                         1            1  
205714286          0 0
             emi_sel                      1            1  
205714286          0 0
                emi                       1            1  
205714286          0 0
       ref_cpu                            1            1  
454736842          0 0
          cpu_pll                         1            1  
454736842          0 0
             cpu                          2            2  
454736842          0 0
                hbus                      4            4  
151578948          0 0
                   fec                    2            2  
151578948          0 0

3) limit ref_io to 288 MHz (= always bit errors on ssp2)

   clock                         enable_cnt  prepare_cnt        rate  
accuracy   phase
----------------------------------------------------------------------------------------
 ref_xtal                                 5            5   
24000000          0 0
    can1                                  0            0   
24000000          0 0
    can0                                  0            0   
24000000          0 0
    uart                                  2            3   
24000000          0 0
    pwm                                   0            0   
24000000          0 0
    rtc                                   0            0      
31250          0 0
    clk32k_div                            1            1      
32000          0 0
       clk32k                             1            1      
32000          0 0
          lradc                           1            1       
2000          0 0
    emi_xtal                              0            0   
24000000          0 0
    xbus                                  3            3   
24000000          0 0
    cpu_xtal                              0            0   
24000000          0 0
    ptp_sel                               0            0   
24000000          0 0
       ptp                                0            0   
24000000          0 0
    lcdif_sel                             0            0   
24000000          0 0
       lcdif_div                          0            0   
24000000          0 0
          lcdif                           0            0   
24000000          0 0
    etm_sel                               0            0   
24000000          0 0
       etm_div                            0            0   
24000000          0 0
          etm                             0            0   
24000000          0 0
    gpmi_sel                              0            0   
24000000          0 0
       gpmi_div                           0            0   
24000000          0 0
          gpmi                            0            0   
24000000          0 0
    pll2                                  1            1   
50000000          0 0
       enet_out                           1            1   
50000000          0 0
    pll1                                  0            0  
480000000          0 0
       usb1_phy                           0            0  
480000000          0 0
          usb1                            0            0  
480000000          0 0
    pll0                                  5            5  
480000000          0 0
       usb0_phy                           2            2  
480000000          0 0
          usb0                            1            1  
480000000          0 0
       spdif_div                          0            0  
120000000          0 0
          spdif                           0            0  
120000000          0 0
       saif1_sel                          0            0  
480000000          0 0
          saif1_div                       0            0       
7324          0 0
             saif1                        0            0       
7324          0 0
       saif0_sel                          0            0  
480000000          0 0
          saif0_div                       0            0       
7324          0 0
             saif0                        0            0       
7324          0 0
       ref_gpmi                           0            0  
480000000          0 0
       ref_hsadc                          0            0  
480000000          0 0
       ref_pix                            0            0  
480000000          0 0
       ref_io0                            1            1  
288000000          0 0
          ssp1_sel                        0            0  
288000000          0 0
             ssp1_div                     0            0  
288000000          0 0
                ssp1                      0            0  
288000000          0 0
          ssp0_sel                        1            1  
288000000          0 0
             ssp0_div                     1            1   
57600000          0 0
                ssp0                      1            1   
57600000          0 0
       ref_io1                            1            1  
288000000          0 0
          ssp3_sel                        0            0  
288000000          0 0
             ssp3_div                     0            0  
288000000          0 0
                ssp3                      0            0  
288000000          0 0
          ssp2_sel                        1            1  
288000000          0 0
             ssp2_div                     1            1  
144000000          0 0
                ssp2                      1            1  
144000000          0 0
       ref_emi                            1            1  
411428571          0 0
          emi_pll                         1            1  
205714286          0 0
             emi_sel                      1            1  
205714286          0 0
                emi                       1            1  
205714286          0 0
       ref_cpu                            1            1  
454736842          0 0
          cpu_pll                         1            1  
454736842          0 0
             cpu                          2            2  
454736842          0 0
                hbus                      4            4  
151578948          0 0
                   fec                    2            2  
151578948          0 0
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Uwe Kleine-König May 5, 2017, 7:25 a.m. UTC | #3
Hello Stefan,

On Thu, May 04, 2017 at 02:25:07PM +0200, Stefan Wahren wrote:
>        ref_io1                            1            1  
> 320000000          0 0

That means your ref_io1 is running at 320 MHz which my patch should
prevent. Are you sure it is applied? What is the register FRAC0
(0x800401b0) set to? Given that ref_io1 is reported as running at
320000000 I'd expect that IO1FRAC is set to 27 which should be
impossible with my patch given that clk_misc_init() in
drivers/clk/mxs/clk-imx28.c initializes IO1FRAC0 to 30 and my patch
should prevent it being set to something smaller than 30.

Something I just noticed now is that the description of FRAC0 includes:

	NOTE: This register can only be addressed by byte instructions.
	Addressing word or half-word are not allowed.

which is not respected by the clock driver.

Best regards
Uwe
Stefan Wahren May 5, 2017, 7:49 a.m. UTC | #4
Hello Uwe,

Am 05.05.2017 um 09:25 schrieb Uwe Kleine-König:
> Hello Stefan,
>
> On Thu, May 04, 2017 at 02:25:07PM +0200, Stefan Wahren wrote:
>>        ref_io1                            1            1  
>> 320000000          0 0
> That means your ref_io1 is running at 320 MHz which my patch should
> prevent. Are you sure it is applied?

sorry for the mess in the dumps.

Your patch works fine regarding to ref_io1. I made 3 dumps:

1. without any patch ( ref_io1 320000000 )
2. with my ref_xtal patch ( ref_io1 288000000 )
3. with your patch ( ref_io1 288000000 )

I assume you looked at the wrong dump.

>  What is the register FRAC0
> (0x800401b0) set to? Given that ref_io1 is reported as running at
> 320000000 I'd expect that IO1FRAC is set to 27 which should be
> impossible with my patch given that clk_misc_init() in
> drivers/clk/mxs/clk-imx28.c initializes IO1FRAC0 to 30 and my patch
> should prevent it being set to something smaller than 30.
>
> Something I just noticed now is that the description of FRAC0 includes:
>
> 	NOTE: This register can only be addressed by byte instructions.
> 	Addressing word or half-word are not allowed.
>
> which is not respected by the clock driver.

I already tried it with no luck. Please follow the discussions [1] and [2].

Maybe the reason why [1] didn't work, was the missing memory barriers?

Please don't get me wrong, i'm very interested in fixing this issue.

Regards Stefan

[1] -
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/318644.html
[2] -
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/323720.html

>
> Best regards
> Uwe
>


--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefan Wahren May 5, 2017, 3:49 p.m. UTC | #5
Am 05.05.2017 um 09:49 schrieb Stefan Wahren:
> Hello Uwe,
>
> Am 05.05.2017 um 09:25 schrieb Uwe Kleine-König:
>> Hello Stefan,
>>
>> On Thu, May 04, 2017 at 02:25:07PM +0200, Stefan Wahren wrote:
>>>        ref_io1                            1            1  
>>> 320000000          0 0
>> That means your ref_io1 is running at 320 MHz which my patch should
>> prevent. Are you sure it is applied?
> sorry for the mess in the dumps.
>
> Your patch works fine regarding to ref_io1. I made 3 dumps:
>
> 1. without any patch ( ref_io1 320000000 )
> 2. with my ref_xtal patch ( ref_io1 288000000 )
> 3. with your patch ( ref_io1 288000000 )
>
> I assume you looked at the wrong dump.
>

Uwe and i made some investigations regards to this issue. The busy bit
of the HW_CLKCTRL_SSP3 was high after boot up, but this shouldn't be the
case. I have the suspicion that there is an issue during clock init or
in the mxs clock driver.

Duckbill (Linux 4.11 Mainline)

HW_SSP2_TIMING    0x80014070 00000209
HW_CLKCTRL_HBUS   0x80040060 00000003
HW_CLKCTRL_SSP0   0x80040090 00000005
HW_CLKCTRL_SSP1   0x800400A0 80000001
HW_CLKCTRL_SSP2   0x800400B0 00000002
HW_CLKCTRL_SSP3   0x800400C0 A0000001
HW_CLKCTRL_EMI    0x800400F0 00000102
HW_CLKCTRL_FRAC0  0x800401B0 5E5B5513
HW_CLKCTRL_CLKSEQ 0x800401D0 00004104

@Uwe: Additionaly applying "spi: mxs: implement runtime pm" to this
patch also doesn't fix the bit error.

Regards Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Uwe Kleine-König May 5, 2017, 8:10 p.m. UTC | #6
Hello,

(adding Michael Krummsdorf who authored a different patch for the same
problem back in 2014 which Stefan found in TQ's patch stack.)

On Fri, May 05, 2017 at 05:49:09PM +0200, Stefan Wahren wrote:
> Uwe and i made some investigations regards to this issue. The busy bit
> of the HW_CLKCTRL_SSP3 was high after boot up, but this shouldn't be the
> case. I have the suspicion that there is an issue during clock init or
> in the mxs clock driver.
> 
> Duckbill (Linux 4.11 Mainline)
> 
> HW_SSP2_TIMING    0x80014070 00000209
> HW_CLKCTRL_HBUS   0x80040060 00000003
> HW_CLKCTRL_SSP0   0x80040090 00000005
> HW_CLKCTRL_SSP1   0x800400A0 80000001
> HW_CLKCTRL_SSP2   0x800400B0 00000002
> HW_CLKCTRL_SSP3   0x800400C0 A0000001
> HW_CLKCTRL_EMI    0x800400F0 00000102
> HW_CLKCTRL_FRAC0  0x800401B0 5E5B5513
> HW_CLKCTRL_CLKSEQ 0x800401D0 00004104

Using barebox as bootloader I have this situation there:

	/ md 0x800400c0+4; md 0x800400c0+4
	800400c0: a0000005                                           ....
	800400c0: a0000005                                           ....

so I suspect the BUSY bit is set when the bootloader is started as
(TTBOMK) barebox didn't touch ssp up to this point. The description of
the busy bit is:

	This read-only bit field returns a one when the clock divider is
	busy transfering a new divider value across clock domains.

That sounds to me as if this bit shouldn't be set for longer than a few
microseconds?!

For me my patch makes the following difference (just checked the above
register set) when Linux is booted:

name              | address    | value before patch | value with patch
------------------+------------+--------------------+-----------------
HW_SSP2_TIMING    | 0x80014070 | 0x00000209         | 0x00000208
HW_CLKCTRL_HBUS   | 0x80040060 | 0x00000003         | 0x00000003
HW_CLKCTRL_SSP0   | 0x80040090 | 0x00000005         | 0x00000005
HW_CLKCTRL_SSP1   | 0x800400a0 | 0x00000005         | 0x00000005
HW_CLKCTRL_SSP2   | 0x800400b0 | 0x80000002         | 0x80000002
HW_CLKCTRL_SSP3   | 0x800400c0 | 0x80000005         | 0xa0000005
HW_CLKCTRL_EMI    | 0x800400f0 | 0x00000102         | 0x00000102
HW_CLKCTRL_FRAC0  | 0x800401b0 | 0x1e9b5513         | 0x5ede5513
HW_CLKCTRL_CLKSEQ | 0x800401d0 | 0x00000104         | 0x00000104

So ref_io1 changed from 480 * (18 / 27) MHz = 320 MHz to
480 * (18 / 30) MHz = 288 MHz (as intended) (FRAC0[16:21]). The other
changes to FRAC0 (IO1_STABLE, IO0_STABLE) don't look relevant?!

Something changed for CLKCTRL_SSP3 (BUSY) even though ssp3 isn't used on
my machine. I also tried to reproduce BUSY disappearing by doing the
same changes to io1 in barebox that Linux does, but without success.

The change to HW_SSP2_TIMING compensates the decreased io1
speed. (Changing the clock rate from

	320 MHz / (2 * 10) = 16 MHz

(which is the target rate setup by the spi-mxs driver) to

	288 MHz / (2 * 9) = 16 MHz

. So it's not actually changed.)

> @Uwe: Additionaly applying "spi: mxs: implement runtime pm" to this
> patch also doesn't fix the bit error.

ok. Would have been nice if that was the relevant difference between
your machine that still has the problem with my patch and mine where the
bug is fixed (or papered over) by my patch.

I wonder if this part of drivers/clk/mxs/clk-imx28.c:

        /*
         * 480 MHz seems too high to be ssp clock source directly,
         * so set frac0 to get a 288 MHz ref_io0 and ref_io1.
         */
        val = readl_relaxed(FRAC0);
        val &= ~((0x3f << BP_FRAC0_IO0FRAC) | (0x3f << BP_FRAC0_IO1FRAC));
        val |= (30 << BP_FRAC0_IO0FRAC) | (30 << BP_FRAC0_IO1FRAC);
        writel_relaxed(val, FRAC0);

is related to this "lowest bit is wrong"-problem and is the result from
a wrong and/or incomplete analysis. It would be nice if someone from NXP
could comment as this code was introduced (for ref_io0 only) in

	7d81397cd93d ("clk: mxs: add clock support for imx28")

by Shawn when he was still employee at Freescale.

Best regards
Uwe
Stefan Wahren May 7, 2017, 11:51 a.m. UTC | #7
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> hat am 5. Mai 2017 um 22:10 geschrieben:
> 
> 
> Hello,
> 
> (adding Michael Krummsdorf who authored a different patch for the same
> problem back in 2014 which Stefan found in TQ's patch stack.)
> 
> On Fri, May 05, 2017 at 05:49:09PM +0200, Stefan Wahren wrote:
> > Uwe and i made some investigations regards to this issue. The busy bit
> > of the HW_CLKCTRL_SSP3 was high after boot up, but this shouldn't be the
> > case. I have the suspicion that there is an issue during clock init or
> > in the mxs clock driver.
> > 
> > Duckbill (Linux 4.11 Mainline)
> > 
> > HW_SSP2_TIMING    0x80014070 00000209
> > HW_CLKCTRL_HBUS   0x80040060 00000003
> > HW_CLKCTRL_SSP0   0x80040090 00000005
> > HW_CLKCTRL_SSP1   0x800400A0 80000001
> > HW_CLKCTRL_SSP2   0x800400B0 00000002
> > HW_CLKCTRL_SSP3   0x800400C0 A0000001
> > HW_CLKCTRL_EMI    0x800400F0 00000102
> > HW_CLKCTRL_FRAC0  0x800401B0 5E5B5513
> > HW_CLKCTRL_CLKSEQ 0x800401D0 00004104
> 
> Using barebox as bootloader I have this situation there:
> 
> 	/ md 0x800400c0+4; md 0x800400c0+4
> 	800400c0: a0000005                                           ....
> 	800400c0: a0000005                                           ....
> 
> so I suspect the BUSY bit is set when the bootloader is started as
> (TTBOMK) barebox didn't touch ssp up to this point. The description of
> the busy bit is:
> 
> 	This read-only bit field returns a one when the clock divider is
> 	busy transfering a new divider value across clock domains.
> 
> That sounds to me as if this bit shouldn't be set for longer than a few
> microseconds?!

This is expected behavior according to the hardware. As long the clock is gated the busy bit will never be cleared. Unfortunately the mxs clk-div doesn't handle this case properly. In case the gate is set, clk-div changes the values and waits that the busy bit become cleared. Since this will never happen, the set_rate operation runs into an unnecessary timeout.

I added a trace_printk into mxs_clk_wait and saw a timeout for changes on ssp3_div. This should be triggered by the clock change of spi-mxs on ssp2_div, because both have the same parent: ref_io1. In order to handle this properly the gate shouldn't be a child of clk-div.

> 
> For me my patch makes the following difference (just checked the above
> register set) when Linux is booted:
> 
> name              | address    | value before patch | value with patch
> ------------------+------------+--------------------+-----------------
> HW_SSP2_TIMING    | 0x80014070 | 0x00000209         | 0x00000208
> HW_CLKCTRL_HBUS   | 0x80040060 | 0x00000003         | 0x00000003
> HW_CLKCTRL_SSP0   | 0x80040090 | 0x00000005         | 0x00000005
> HW_CLKCTRL_SSP1   | 0x800400a0 | 0x00000005         | 0x00000005
> HW_CLKCTRL_SSP2   | 0x800400b0 | 0x80000002         | 0x80000002
> HW_CLKCTRL_SSP3   | 0x800400c0 | 0x80000005         | 0xa0000005
> HW_CLKCTRL_EMI    | 0x800400f0 | 0x00000102         | 0x00000102
> HW_CLKCTRL_FRAC0  | 0x800401b0 | 0x1e9b5513         | 0x5ede5513
> HW_CLKCTRL_CLKSEQ | 0x800401d0 | 0x00000104         | 0x00000104
> 
> So ref_io1 changed from 480 * (18 / 27) MHz = 320 MHz to
> 480 * (18 / 30) MHz = 288 MHz (as intended) (FRAC0[16:21]). The other
> changes to FRAC0 (IO1_STABLE, IO0_STABLE) don't look relevant?!
> 

I have no idea because those flags only toggle on success.

> 
> I wonder if this part of drivers/clk/mxs/clk-imx28.c:
> 
>         /*
>          * 480 MHz seems too high to be ssp clock source directly,
>          * so set frac0 to get a 288 MHz ref_io0 and ref_io1.
>          */
>         val = readl_relaxed(FRAC0);
>         val &= ~((0x3f << BP_FRAC0_IO0FRAC) | (0x3f << BP_FRAC0_IO1FRAC));
>         val |= (30 << BP_FRAC0_IO0FRAC) | (30 << BP_FRAC0_IO1FRAC);
>         writel_relaxed(val, FRAC0);
> 
> is related to this "lowest bit is wrong"-problem and is the result from
> a wrong and/or incomplete analysis. 

I think there are 2 possible issues with this init code. First we try to set IO0FRAC and IO1FRAC at the same time.

The other one is the order. The parent of all 4 SSP clocks are switched to ref_io, before the FRAC0 register is set up to 288 MHz. This shouldn't be a real problem as long as the bootloader init FRAC0 before.

Nevertheless i prepare a proof of concept (hacky) patch to address all 3 possible issues [1]:
- swap FRAC0 setup and CLKSEQ bypass changes for all SSPs
- split IO0FRAC and IO1FRAC changes into 2 separate R-M-W sequences incl. memory barrier
- avoid clock changes to sspX_div if they are gated or busy
- additionaly trace_printks

I tested it with Duckbill, but without a QCA7000. So i don't know if it fixes the bit errors with the QCA7000.

Uwe, could you please test it with your EEPROM but without your 2 clock patches?
Does it have any influence on the bit errors?

[1] - https://gist.github.com/lategoodbye/4db0ad08d1c23dfac51115d206ecbd7a
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Uwe Kleine-König May 8, 2017, 8:24 a.m. UTC | #8
Hello Stefan,

On Sun, May 07, 2017 at 01:51:54PM +0200, Stefan Wahren wrote:
> > On Fri, May 05, 2017 at 05:49:09PM +0200, Stefan Wahren wrote:
> > > Uwe and i made some investigations regards to this issue. The busy bit
> > > of the HW_CLKCTRL_SSP3 was high after boot up, but this shouldn't be the
> > > case. I have the suspicion that there is an issue during clock init or
> > > in the mxs clock driver.
> > > 
> > > Duckbill (Linux 4.11 Mainline)
> > > 
> > > HW_SSP2_TIMING    0x80014070 00000209
> > > HW_CLKCTRL_HBUS   0x80040060 00000003
> > > HW_CLKCTRL_SSP0   0x80040090 00000005
> > > HW_CLKCTRL_SSP1   0x800400A0 80000001
> > > HW_CLKCTRL_SSP2   0x800400B0 00000002
> > > HW_CLKCTRL_SSP3   0x800400C0 A0000001
> > > HW_CLKCTRL_EMI    0x800400F0 00000102
> > > HW_CLKCTRL_FRAC0  0x800401B0 5E5B5513
> > > HW_CLKCTRL_CLKSEQ 0x800401D0 00004104
> > 
> > Using barebox as bootloader I have this situation there:
> > 
> > 	/ md 0x800400c0+4; md 0x800400c0+4
> > 	800400c0: a0000005                                           ....
> > 	800400c0: a0000005                                           ....
> > 
> > so I suspect the BUSY bit is set when the bootloader is started as
> > (TTBOMK) barebox didn't touch ssp up to this point. The description of
> > the busy bit is:
> > 
> > 	This read-only bit field returns a one when the clock divider is
> > 	busy transfering a new divider value across clock domains.
> > 
> > That sounds to me as if this bit shouldn't be set for longer than a few
> > microseconds?!
> 
> This is expected behavior according to the hardware. As long the clock is gated the busy bit will never be cleared. Unfortunately the mxs clk-div doesn't handle this case properly. In case the gate is set, clk-div changes the values and waits that the busy bit become cleared. Since this will never happen, the set_rate operation runs into an unnecessary timeout.
> 
> I added a trace_printk into mxs_clk_wait and saw a timeout for changes on ssp3_div. This should be triggered by the clock change of spi-mxs on ssp2_div, because both have the same parent: ref_io1. In order to handle this properly the gate shouldn't be a child of clk-div.
> 
> > 
> > For me my patch makes the following difference (just checked the above
> > register set) when Linux is booted:
> > 
> > name              | address    | value before patch | value with patch
> > ------------------+------------+--------------------+-----------------
> > HW_SSP2_TIMING    | 0x80014070 | 0x00000209         | 0x00000208
> > HW_CLKCTRL_HBUS   | 0x80040060 | 0x00000003         | 0x00000003
> > HW_CLKCTRL_SSP0   | 0x80040090 | 0x00000005         | 0x00000005
> > HW_CLKCTRL_SSP1   | 0x800400a0 | 0x00000005         | 0x00000005
> > HW_CLKCTRL_SSP2   | 0x800400b0 | 0x80000002         | 0x80000002
> > HW_CLKCTRL_SSP3   | 0x800400c0 | 0x80000005         | 0xa0000005
> > HW_CLKCTRL_EMI    | 0x800400f0 | 0x00000102         | 0x00000102
> > HW_CLKCTRL_FRAC0  | 0x800401b0 | 0x1e9b5513         | 0x5ede5513
> > HW_CLKCTRL_CLKSEQ | 0x800401d0 | 0x00000104         | 0x00000104
> > 
> > So ref_io1 changed from 480 * (18 / 27) MHz = 320 MHz to
> > 480 * (18 / 30) MHz = 288 MHz (as intended) (FRAC0[16:21]). The other
> > changes to FRAC0 (IO1_STABLE, IO0_STABLE) don't look relevant?!
> > 
> 
> I have no idea because those flags only toggle on success.
> 
> > 
> > I wonder if this part of drivers/clk/mxs/clk-imx28.c:
> > 
> >         /*
> >          * 480 MHz seems too high to be ssp clock source directly,
> >          * so set frac0 to get a 288 MHz ref_io0 and ref_io1.
> >          */
> >         val = readl_relaxed(FRAC0);
> >         val &= ~((0x3f << BP_FRAC0_IO0FRAC) | (0x3f << BP_FRAC0_IO1FRAC));
> >         val |= (30 << BP_FRAC0_IO0FRAC) | (30 << BP_FRAC0_IO1FRAC);
> >         writel_relaxed(val, FRAC0);
> > 
> > is related to this "lowest bit is wrong"-problem and is the result from
> > a wrong and/or incomplete analysis. 
> 
> I think there are 2 possible issues with this init code. First we try to set IO0FRAC and IO1FRAC at the same time.
> 
> The other one is the order. The parent of all 4 SSP clocks are switched to ref_io, before the FRAC0 register is set up to 288 MHz. This shouldn't be a real problem as long as the bootloader init FRAC0 before.
> 
> Nevertheless i prepare a proof of concept (hacky) patch to address all 3 possible issues [1]:
> - swap FRAC0 setup and CLKSEQ bypass changes for all SSPs
> - split IO0FRAC and IO1FRAC changes into 2 separate R-M-W sequences incl. memory barrier
> - avoid clock changes to sspX_div if they are gated or busy
> - additionaly trace_printks
> 
> I tested it with Duckbill, but without a QCA7000. So i don't know if it fixes the bit errors with the QCA7000.
> 
> Uwe, could you please test it with your EEPROM but without your 2 clock patches?
> Does it have any influence on the bit errors?

The patch doesn't make the bit errors go away. The only message being
generated is:

	swapper-1     [000] .......     1.134985: clk_div_set_rate: Cannot to set ssp3_div: gated 

When I compare your clk_summary (with my patch) to my working one the
only differences are some enable/prepare counts, you have the following
clocks on while I have not:

	lradc, enet_out, lcdif_sel (also reparented), fec

while I have these clocks on and you have not:

	ssp1

There are no differences in clk freqs for clocks that we both have on
which should rule out a problem of competing clock domains.

I'm out of ideas :-(

Best regards
Uwe
Krummsdorf Michael May 8, 2017, 9:53 a.m. UTC | #9
Hello Uwe, 

> (adding Michael Krummsdorf who authored a different patch for the same
> problem back in 2014 which Stefan found in TQ's patch stack.)

thanks for adding me.
Back then I narrowed the issue down to two conditions:
* ref_io had to be clock source and
* ssp clock div had to be 1

Limiting ssp clock div to be at least 2 eliminated the error.
So in the driver I capped clk_freq to 480 MHz / 2.
I did not consider changing ref_io frequency.

Best regards,
i.A. Michael Krummsdorf
Entwicklung Standort Leipzig
Tel. +49 341 308598-27, Fax +49 341 308598-11,
Friedrich-List-Platz 2, 04103 Leipzig
mailto:michael.krummsdorf@tq-group.com
 
TQ-Systems GmbH
Mühlstraße 2, Gut Delling, 82229 Seefeld
Amtsgericht München, HRB 105018
Sitz der Gesellschaft: Seefeld
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
www.tq-group.com

Besuchen Sie uns:
Intersolar Europe, München, 31. Mai - 2. Juni 2017, Halle B2, Stand B2.234
MT-CONNECT, Nürnberg, 21. - 22. Juni 2017, Halle 10.1, Stand 10.1-116
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Uwe Kleine-König May 8, 2017, 10:14 a.m. UTC | #10
Hello,

On Mon, May 08, 2017 at 09:53:28AM +0000, Krummsdorf Michael wrote:
> > (adding Michael Krummsdorf who authored a different patch for the same
> > problem back in 2014 which Stefan found in TQ's patch stack.)
> 
> thanks for adding me.
> Back then I narrowed the issue down to two conditions:
> * ref_io had to be clock source and
> * ssp clock div had to be 1

This is not the complete problem description because my patch lowers the
speed on ref_io and has no influence on HW_CLKCTRL_SSP2 (which has
already DIV=2 for me before the patch where it's broken).

Also Stefan has values >1 and the problem is still there on a duckbill.

Best regards
Uwe
Stefan Wahren May 10, 2017, 1:39 p.m. UTC | #11
Am 08.05.2017 um 10:24 schrieb Uwe Kleine-König:
> Hello Stefan,
>
> The patch doesn't make the bit errors go away. The only message being
> generated is:
>
> 	swapper-1     [000] .......     1.134985: clk_div_set_rate: Cannot to set ssp3_div: gated 
>
> When I compare your clk_summary (with my patch) to my working one the
> only differences are some enable/prepare counts, you have the following
> clocks on while I have not:
>
> 	lradc, enet_out, lcdif_sel (also reparented), fec
>
> while I have these clocks on and you have not:
>
> 	ssp1

Sorry, i don't have the time for any further investigations. Out of
curiosity, what is connected to the ssp1 on your board?

Stefan

>
> There are no differences in clk freqs for clocks that we both have on
> which should rule out a problem of competing clock domains.
>
> I'm out of ideas :-(
>
> Best regards
> Uwe
>


--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Uwe Kleine-König May 10, 2017, 2:13 p.m. UTC | #12
Hello Stefan,

On Wed, May 10, 2017 at 03:39:14PM +0200, Stefan Wahren wrote:
> Am 08.05.2017 um 10:24 schrieb Uwe Kleine-König:
> > The patch doesn't make the bit errors go away. The only message being
> > generated is:
> >
> > 	swapper-1     [000] .......     1.134985: clk_div_set_rate: Cannot to set ssp3_div: gated 
> >
> > When I compare your clk_summary (with my patch) to my working one the
> > only differences are some enable/prepare counts, you have the following
> > clocks on while I have not:
> >
> > 	lradc, enet_out, lcdif_sel (also reparented), fec
> >
> > while I have these clocks on and you have not:
> >
> > 	ssp1

	ssp0: SD-card
	ssp1: eMMC (which /)
	ssp2: eeprom

Uwe
Uwe Kleine-König May 10, 2017, 6:05 p.m. UTC | #13
Hello,

I found something strange while playing around with the
HW_CLKCTRL_FRAC0 register:

	# memtool md 0x800401b0+4
	800401b0: 0e1d5513                                           .U..
	# memtool mw 0x800401b0 0x0e1d5513
	# memtool md 0x800401b0+4
	800401b0: 4e5d5513                                           .U]N

Huh, why did IO0_STABLE and IO1_STABLE toggle? OK, after all we should access
this register using 8-bit accesses:

	# memtool md -b 0x800401b0+4
	800401b0: 13 55 5d 4e                                        .U]N
	# memtool mw -b 0x800401b2 0x5d
	# memtool md -b 0x800401b0+4
	800401b0: 13 55 5d 4e                                        .U]N

hmm, now IO1_STABLE didn't invert.

	# memtool mw -b 0x800401b2 0x5c
	# memtool md -b 0x800401b0+4
	800401b0: 13 55 5c 4e                                        .U\N

not even now even though IO1FRAC changed!

	# memtool md  0x800401b0+4
	800401b0: 4e5c5513                                           .U\N
	# memtool mw -b 0x800401b2 0x5b
	# memtool md 0x800401b0+4
	800401b0: 4e5b5513                                           .U[N

again IO1_STABLE doesn't toggle (this time with 32-bit access)

	# memtool mw  0x800401b0 0x4e5b5513
	# memtool md  0x800401b0+4
	800401b0: 0e1b5513                                           .U..

Just rewriting the reported value makes both IO0_STABLE and IO1_STABLE toggle.

Definitely strange.

Best regards
Uwe
Uwe Kleine-König May 10, 2017, 8:26 p.m. UTC | #14
Hello,

	# cat dumpclk 
	#!/bin/sh

	for r in 0x80014070 0x80040060 0x80040090 0x800400A0 0x800400B0 0x800400C0 0x800400F0 0x800401B0 0x800401D0; do
		memtool md $r+4
	done

	# cat dumpeeprom 
	#!/bin/sh

	dd if=/sys/bus/spi/devices/spi1.0/eeprom bs=16 count=1 2>/dev/null | hexdump -C

Now when booting with an unpatched 4.9 kernel I have after startup:

	# ./dumpclk 
	80014070: 00000204                                           ....
	80040060: 00000003                                           ....
	80040090: 00000005                                           ....
	800400a0: 00000005                                           ....
	800400b0: 00000002                                           ....
	800400c0: 80000005                                           ....
	800400f0: 00000102                                           ....
	800401b0: 5e5b5513                                           .U[^
	800401d0: 00000104                                           ....
	# ./dumpeeprom 
	00000000  08 ab 00 00 5f 70 00 00  00 80 00 03 00 00 cb 00  |...._p..........|
	00000010

(which is broken, because the first byte in the eeprom is a 0x09).

Now I make ref_io slower again:

	# memtool mw 0x800401b0 0x5e5e5513
	# ./dumpclk
	80014070: 00000204                                           ....
	80040060: 00000003                                           ....
	80040090: 00000005                                           ....
	800400a0: 00000005                                           ....
	800400b0: 00000002                                           ....
	800400c0: 80000005                                           ....
	800400f0: 00000102                                           ....
	800401b0: 1e1e5513                                           .U^.
	800401d0: 00000104                                           ....
	# ./dumpeeprom
	00000000  09 aa 00 00 5f 70 00 00  00 80 00 02 00 00 ca 00  |...._p..........|
	00000010

So on my machine it really seems to only depend on ref-io1 clk speed.
With all available slower speeds resulting from IOFRAC1 in [0x1f ..
0x23] accessing the eeprom works fine. With IOFRAC = 0x1d reading works
fine already. With all lower values [0x12 .. 0x1c] reading shows these
bit errors. Also more or less randomly jumping between different
settings for IO1FRAC doesn't suggest that there might be something else
influencing the bug. It was always [0x12 .. 0x1c] shows the bug, [0x1d
.. 0x23] doesn't.

I also did a test using different values for SSP2_TIMING.CLOCK_DIVIDE
and SSP2_TIMING.CLOCK_RATE. Going through all 32504 settings that don't
yield a bus freq > 16 MHz with ssp2-clk running at 144 MHz or 160 MHz
doesn't show any variation. It's always fine on 144 MHz (IOFRAC = 0x1e)
and always fails on 160 MHz (IOFRAC = 0x1b).

Best regards
Uwe
Uwe Kleine-König May 26, 2017, 12:06 p.m. UTC | #15
Hello,

On Wed, May 03, 2017 at 04:53:09PM -0300, Fabio Estevam wrote:
> On Wed, May 3, 2017 at 3:56 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and
> > 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for
> > SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized
> > to 288 MHz because the initial frequency "seems too high to be ssp clock
> > source directly". However this isn't enough to ensure that the frequency
> > isn't increased later again. In fact this happens on my machine as the
> > mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being
> > ssp2 which is resolved to
> >
> >         ref_io1.rate = 320 MHz
> >         ssp2_sel.parent = ref_io1
> >         ssp2_div.divider = 2
> >
> > . The observed effect is that reading MISO reliably fails: Instead of
> > the least significant bit the second least significant bit is reported
> > twice. This is probably related to the reports
> >
> >         https://community.nxp.com/thread/290209
> >         https://community.nxp.com/thread/310434
> 
> Adding Stefan on Cc in he could help testing this patch, as he was the
> one that created this thread.

Even though my patch doesn't help Stefan, I think it should be applied
nevertheless as it makes my machine work, just completes the
workaround/fix that is already in mainline and nobody who still has the
issue has the time to debug this further.

Best regards
Uwe
Stefan Wahren May 29, 2017, 9:06 p.m. UTC | #16
Hi Uwe,

> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> hat am 26. Mai 2017 um 14:06 geschrieben:
> 
> 
> Hello,
> 
> On Wed, May 03, 2017 at 04:53:09PM -0300, Fabio Estevam wrote:
> > On Wed, May 3, 2017 at 3:56 PM, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and
> > > 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for
> > > SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized
> > > to 288 MHz because the initial frequency "seems too high to be ssp clock
> > > source directly". However this isn't enough to ensure that the frequency
> > > isn't increased later again. In fact this happens on my machine as the
> > > mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being
> > > ssp2 which is resolved to
> > >
> > >         ref_io1.rate = 320 MHz
> > >         ssp2_sel.parent = ref_io1
> > >         ssp2_div.divider = 2
> > >
> > > . The observed effect is that reading MISO reliably fails: Instead of
> > > the least significant bit the second least significant bit is reported
> > > twice. This is probably related to the reports
> > >
> > >         https://community.nxp.com/thread/290209
> > >         https://community.nxp.com/thread/310434
> > 
> > Adding Stefan on Cc in he could help testing this patch, as he was the
> > one that created this thread.
> 
> Even though my patch doesn't help Stefan, I think it should be applied
> nevertheless as it makes my machine work, just completes the
> workaround/fix that is already in mainline and nobody who still has the
> issue has the time to debug this further.

i like to see at least a comment within the code that the change is a workaround.

Thanks
Stefan

> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam May 29, 2017, 9:21 p.m. UTC | #17
Hi Uwe,

On Wed, May 3, 2017 at 3:56 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and
> 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for
> SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized
> to 288 MHz because the initial frequency "seems too high to be ssp clock
> source directly". However this isn't enough to ensure that the frequency
> isn't increased later again. In fact this happens on my machine as the
> mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being
> ssp2 which is resolved to
>
>         ref_io1.rate = 320 MHz
>         ssp2_sel.parent = ref_io1
>         ssp2_div.divider = 2
>
> . The observed effect is that reading MISO reliably fails: Instead of
> the least significant bit the second least significant bit is reported
> twice. This is probably related to the reports
>
>         https://community.nxp.com/thread/290209
>         https://community.nxp.com/thread/310434

As Stefan mentioned that your patch does not solve his issue, please
remove this link when you submit a v2.
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Uwe Kleine-König May 30, 2017, 6:54 a.m. UTC | #18
On Mon, May 29, 2017 at 06:21:49PM -0300, Fabio Estevam wrote:
> Hi Uwe,
> 
> On Wed, May 3, 2017 at 3:56 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and
> > 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for
> > SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized
> > to 288 MHz because the initial frequency "seems too high to be ssp clock
> > source directly". However this isn't enough to ensure that the frequency
> > isn't increased later again. In fact this happens on my machine as the
> > mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being
> > ssp2 which is resolved to
> >
> >         ref_io1.rate = 320 MHz
> >         ssp2_sel.parent = ref_io1
> >         ssp2_div.divider = 2
> >
> > . The observed effect is that reading MISO reliably fails: Instead of
> > the least significant bit the second least significant bit is reported
> > twice. This is probably related to the reports
> >
> >         https://community.nxp.com/thread/290209
> >         https://community.nxp.com/thread/310434
> 
> As Stefan mentioned that your patch does not solve his issue, please
> remove this link when you submit a v2.

I still think it is related to these reports, don't you?

Best regards
Uwe
Stefan Wahren May 30, 2017, 8:04 a.m. UTC | #19
Am 30.05.2017 um 08:54 schrieb Uwe Kleine-König:
> On Mon, May 29, 2017 at 06:21:49PM -0300, Fabio Estevam wrote:
>> Hi Uwe,
>>
>> On Wed, May 3, 2017 at 3:56 PM, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>>> Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and
>>> 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for
>>> SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized
>>> to 288 MHz because the initial frequency "seems too high to be ssp clock
>>> source directly". However this isn't enough to ensure that the frequency
>>> isn't increased later again. In fact this happens on my machine as the
>>> mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being
>>> ssp2 which is resolved to
>>>
>>>         ref_io1.rate = 320 MHz
>>>         ssp2_sel.parent = ref_io1
>>>         ssp2_div.divider = 2
>>>
>>> . The observed effect is that reading MISO reliably fails: Instead of
>>> the least significant bit the second least significant bit is reported
>>> twice. This is probably related to the reports
>>>
>>>         https://community.nxp.com/thread/290209
>>>         https://community.nxp.com/thread/310434
>> As Stefan mentioned that your patch does not solve his issue, please
>> remove this link when you submit a v2.
> I still think it is related to these reports, don't you?

I think this is related somehow, but this version doesn't mention that
it doesn't fix the linked issue. So removing the link would be the best
in order to avoid confusion.

Btw did you tried to disable DMA transfer during your tests?

Regards
Stefan

>
> Best regards
> Uwe
>


--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam May 30, 2017, 11:13 a.m. UTC | #20
On Tue, May 30, 2017 at 3:54 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:

> I still think it is related to these reports, don't you?

Your commit log gives the impression that your patch fixes Stefan's
reported problem, which can be confusing.
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Uwe Kleine-König July 26, 2018, 2:32 p.m. UTC | #21
On Wed, May 03, 2017 at 08:56:25PM +0200, Uwe Kleine-König wrote:
> Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and
> 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for
> SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized
> to 288 MHz because the initial frequency "seems too high to be ssp clock
> source directly". However this isn't enough to ensure that the frequency
> isn't increased later again. In fact this happens on my machine as the
> mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being
> ssp2 which is resolved to
> 
> 	ref_io1.rate = 320 MHz
> 	ssp2_sel.parent = ref_io1
> 	ssp2_div.divider = 2
> 
> . The observed effect is that reading MISO reliably fails: Instead of
> the least significant bit the second least significant bit is reported
> twice. This is probably related to the reports
> 
> 	https://community.nxp.com/thread/290209
> 	https://community.nxp.com/thread/310434
> 	https://community.nxp.com/thread/385546
> 
> So additionally to initializing ref_io0 and ref_io1 to 288 MHz ensure
> that their frequency is never set to something bigger later on. This is
> done by adding a parameter min_frac to clk-ref and use that instead of
> the hard coded 18 to limit the valid values for FRAC. For all ref clocks
> but ref_io0 and ref_io1 18 is used and so there is no functional change
> for those.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Back in 2017 this patch made the issue disappear on a customer's machine
(using TQ's TQMa28). Now the problem is back on a variant of the machine
(using TQMa28L). Back then Stefan had the problem with my patch on a
Duckbill, too.

Here you can see the problem in action:

I hava a file that contains just the bytes from 0 to 256:

	root@em-switch:~ hexdump -C lala
	00000000  00 01 02 03 04 05 06 07  08 09 0a 0b 0c 0d 0e 0f  |................|
	00000010  10 11 12 13 14 15 16 17  18 19 1a 1b 1c 1d 1e 1f  |................|
	00000020  20 21 22 23 24 25 26 27  28 29 2a 2b 2c 2d 2e 2f  | !"#$%&'()*+,-./|
	00000030  30 31 32 33 34 35 36 37  38 39 3a 3b 3c 3d 3e 3f  |0123456789:;<=>?|
	00000040  40 41 42 43 44 45 46 47  48 49 4a 4b 4c 4d 4e 4f  |@ABCDEFGHIJKLMNO|
	00000050  50 51 52 53 54 55 56 57  58 59 5a 5b 5c 5d 5e 5f  |PQRSTUVWXYZ[\]^_|
	00000060  60 61 62 63 64 65 66 67  68 69 6a 6b 6c 6d 6e 6f  |`abcdefghijklmno|
	00000070  70 71 72 73 74 75 76 77  78 79 7a 7b 7c 7d 7e 7f  |pqrstuvwxyz{|}~.|
	00000080  80 81 82 83 84 85 86 87  88 89 8a 8b 8c 8d 8e 8f  |................|
	00000090  90 91 92 93 94 95 96 97  98 99 9a 9b 9c 9d 9e 9f  |................|
	000000a0  a0 a1 a2 a3 a4 a5 a6 a7  a8 a9 aa ab ac ad ae af  |................|
	000000b0  b0 b1 b2 b3 b4 b5 b6 b7  b8 b9 ba bb bc bd be bf  |................|
	000000c0  c0 c1 c2 c3 c4 c5 c6 c7  c8 c9 ca cb cc cd ce cf  |................|
	000000d0  d0 d1 d2 d3 d4 d5 d6 d7  d8 d9 da db dc dd de df  |................|
	000000e0  e0 e1 e2 e3 e4 e5 e6 e7  e8 e9 ea eb ec ed ee ef  |................|
	000000f0  f0 f1 f2 f3 f4 f5 f6 f7  f8 f9 fa fb fc fd fe     |...............|
	000000ff

this was written to an spi eeprom. Reading it looks ok on the
oscilloscope, but in the driver I get the described read errors:

	root@em-switch:~ cmp -l lala /sys/bus/spi/devices/spi1.0/eeprom
	  3   2   3
	 15  16  17
	 18  21  20
	 23  26  27
	 34  41  40
	 35  42  43
	 38  45  44
	 46  55  54
	 54  65  64
	 87 126 127
	143 216 217
	199 306 307
	203 312 313
	211 322 323
	231 346 347
	239 356 357
	243 362 363
	251 372 373
	cmp: EOF on lala
	root@em-switch:~ cmp -l lala /sys/bus/spi/devices/spi1.0/eeprom
	 10  11  10
	 11  12  13
	 18  21  20
	 23  26  27
	 26  31  30
	 38  45  44
	 39  46  47
	 47  56  57
	 59  72  73
	114 161 160
	151 226 227
	179 262 263
	203 312 313
	239 356 357
	247 366 367
	251 372 373
	255 376 377
	cmp: EOF on lala
	root@em-switch:~ cmp -l lala /sys/bus/spi/devices/spi1.0/eeprom
	 15  16  17
	 19  22  23
	 27  32  33
	 35  42  43
	 51  62  63
	 55  66  67
	 63  76  77
	 71 106 107
	 79 116 117
	 83 122 123
	 87 126 127
	 91 132 133
	 95 136 137
	 99 142 143
	123 172 173
	138 211 210
	139 212 213
	151 226 227
	155 232 233
	163 242 243
	166 245 244
	187 272 273
	231 346 347
	cmp: EOF on lala

(The output of cmp -l is for each differing byte: The decimal byte number
(starting at 1), and then in octal the two values.) As you can see the
problem is on changing offsets and the value in the third column always
only differs in the least-significant bit and the wrong values always
end in 0, 3, 4 or 7, so the two least-significant bits are always
identical.

So it seems the last bit (which is the last on the bus, too) is sampled
too early by the CPU.

Is the problem better understood in the meantime by someone? It would be
great to find and fix the underlying issue. I guess only someone with a
view into the SoC can give definitive results, though.
Fabio: Doesn't that look bad enough to let the hardware guys at NXP
look into that?

Best regards
Uwe
Stefan Wahren July 26, 2018, 2:50 p.m. UTC | #22
Hi Uwe,

no new insights from my side.

Am 26.07.2018 um 16:32 schrieb Uwe Kleine-König:
> On Wed, May 03, 2017 at 08:56:25PM +0200, Uwe Kleine-König wrote:
>> Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and
>> 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for
>> SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized
>> to 288 MHz because the initial frequency "seems too high to be ssp clock
>> source directly". However this isn't enough to ensure that the frequency
>> isn't increased later again. In fact this happens on my machine as the
>> mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being
>> ssp2 which is resolved to
>>
>> 	ref_io1.rate = 320 MHz
>> 	ssp2_sel.parent = ref_io1
>> 	ssp2_div.divider = 2
>>
>> . The observed effect is that reading MISO reliably fails: Instead of
>> the least significant bit the second least significant bit is reported
>> twice. This is probably related to the reports
>>
>> 	https://community.nxp.com/thread/290209
>> 	https://community.nxp.com/thread/310434
>> 	https://community.nxp.com/thread/385546
>>
>> So additionally to initializing ref_io0 and ref_io1 to 288 MHz ensure
>> that their frequency is never set to something bigger later on. This is
>> done by adding a parameter min_frac to clk-ref and use that instead of
>> the hard coded 18 to limit the valid values for FRAC. For all ref clocks
>> but ref_io0 and ref_io1 18 is used and so there is no functional change
>> for those.
>>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Back in 2017 this patch made the issue disappear on a customer's machine
> (using TQ's TQMa28). Now the problem is back on a variant of the machine
> (using TQMa28L). Back then Stefan had the problem with my patch on a
> Duckbill, too.
>

I only want to note that we are using this ugly hack for Duckbill to 
avoid this issue:

https://github.com/I2SE/linux/commit/e457d5e0309c07e30e8dd8b17c32f7b3cbdd9547

Best regards
Stefan

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam July 26, 2018, 3:02 p.m. UTC | #23
Hi Uwe,

On Thu, Jul 26, 2018 at 11:50 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote:

> I only want to note that we are using this ugly hack for Duckbill to avoid
> this issue:
>
> https://github.com/I2SE/linux/commit/e457d5e0309c07e30e8dd8b17c32f7b3cbdd9547

Do you see the problem on TQMa28L with Stefan's hack applied?
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam July 26, 2018, 3:04 p.m. UTC | #24
Hi Uwe,

On Thu, Jul 26, 2018 at 11:32 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:

> Fabio: Doesn't that look bad enough to let the hardware guys at NXP
> look into that?

Yes, I think we should ask for their help on this problem.
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam July 26, 2018, 3:18 p.m. UTC | #25
Hi Uwe,

On Thu, Jul 26, 2018 at 12:04 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Uwe,
>
> On Thu, Jul 26, 2018 at 11:32 AM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
>
>> Fabio: Doesn't that look bad enough to let the hardware guys at NXP
>> look into that?
>
> Yes, I think we should ask for their help on this problem.

We are changing the ref_io while it is already feeding SSP, which is
not a good practice.

It would be better if we could change the ref_io frequency first, and
then feed the SSP clock afterwards:

--- a/drivers/clk/mxs/clk-imx28.c
+++ b/drivers/clk/mxs/clk-imx28.c
@@ -110,12 +110,6 @@ static void __init clk_misc_init(void)
        writel_relaxed(val, ENET);

        /*
-        * Source ssp clock from ref_io than ref_xtal,
-        * as ref_xtal only provides 24 MHz as maximum.
-        */
-       writel_relaxed(0xf << BP_CLKSEQ_BYPASS_SSP0, CLKSEQ + CLR);
-
-       /*
         * 480 MHz seems too high to be ssp clock source directly,
         * so set frac0 to get a 288 MHz ref_io0 and ref_io1.
         */
@@ -123,6 +117,12 @@ static void __init clk_misc_init(void)
        val &= ~((0x3f << BP_FRAC0_IO0FRAC) | (0x3f << BP_FRAC0_IO1FRAC));
        val |= (30 << BP_FRAC0_IO0FRAC) | (30 << BP_FRAC0_IO1FRAC);
        writel_relaxed(val, FRAC0);
+
+       /*
+        * Source ssp clock from ref_io than ref_xtal,
+        * as ref_xtal only provides 24 MHz as maximum.
+        */
+       writel_relaxed(0xf << BP_CLKSEQ_BYPASS_SSP0, CLKSEQ + CLR);
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Uwe Kleine-König Aug. 1, 2018, 9:31 a.m. UTC | #26
Hello Fabio,

On Thu, Jul 26, 2018 at 12:02:42PM -0300, Fabio Estevam wrote:
> On Thu, Jul 26, 2018 at 11:50 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> 
> > I only want to note that we are using this ugly hack for Duckbill to avoid
> > this issue:
> >
> > https://github.com/I2SE/linux/commit/e457d5e0309c07e30e8dd8b17c32f7b3cbdd9547
> 
> Do you see the problem on TQMa28L with Stefan's hack applied?

With this change by Stefan the bug is not reproducible. Your change to
set the bypass bits only later doesn't help though.

Best regards
Uwe
Uwe Kleine-König Aug. 2, 2018, 8:33 a.m. UTC | #27
Hello,

On Wed, Aug 01, 2018 at 11:31:13AM +0200, Uwe Kleine-König wrote:
> On Thu, Jul 26, 2018 at 12:02:42PM -0300, Fabio Estevam wrote:
> > On Thu, Jul 26, 2018 at 11:50 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> > 
> > > I only want to note that we are using this ugly hack for Duckbill to avoid
> > > this issue:
> > >
> > > https://github.com/I2SE/linux/commit/e457d5e0309c07e30e8dd8b17c32f7b3cbdd9547
> > 
> > Do you see the problem on TQMa28L with Stefan's hack applied?
> 
> With this change by Stefan the bug is not reproducible. Your change to
> set the bypass bits only later doesn't help though.

I wanted to try without DMA (by just removing dem dma properties in the
mmc node) but this makes the driver fail to probe :-|

Best regards
Uwe
Uwe Kleine-König Aug. 3, 2018, 9:09 a.m. UTC | #28
On Thu, Aug 02, 2018 at 10:33:18AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Aug 01, 2018 at 11:31:13AM +0200, Uwe Kleine-König wrote:
> > On Thu, Jul 26, 2018 at 12:02:42PM -0300, Fabio Estevam wrote:
> > > On Thu, Jul 26, 2018 at 11:50 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> > > 
> > > > I only want to note that we are using this ugly hack for Duckbill to avoid
> > > > this issue:
> > > >
> > > > https://github.com/I2SE/linux/commit/e457d5e0309c07e30e8dd8b17c32f7b3cbdd9547
> > > 
> > > Do you see the problem on TQMa28L with Stefan's hack applied?
> > 
> > With this change by Stefan the bug is not reproducible. Your change to
> > set the bypass bits only later doesn't help though.
> 
> I wanted to try without DMA (by just removing dem dma properties in the
> mmc node) but this makes the driver fail to probe :-|

I tested with 

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 3d216b950b41..36374e71d80a 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -401,7 +401,7 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 		 * DMA only: 2.164808 seconds, 473.0KB/s
 		 * Combined: 1.676276 seconds, 610.9KB/s
 		 */
-		if (t->len < 32) {
+		if (t->len < 32 || 1) {
 			writel(BM_SSP_CTRL1_DMA_ENABLE,
 				ssp->base + HW_SSP_CTRL1(ssp) +
 				STMP_OFFSET_REG_CLR);

which effectively disables DMA, the problem is still there. So DMA can
be ruled out.

Best regards
Uwe
Stefan Wahren Aug. 8, 2018, 8:23 a.m. UTC | #29
Hi,

> > On Thu, Jul 26, 2018 at 11:50 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> > 
> > > I only want to note that we are using this ugly hack for Duckbill to avoid
> > > this issue:
> > >
> > > https://github.com/I2SE/linux/commit/e457d5e0309c07e30e8dd8b17c32f7b3cbdd9547
> > 
> > Do you see the problem on TQMa28L with Stefan's hack applied?
> 
> With this change by Stefan the bug is not reproducible. Your change to
> set the bypass bits only later doesn't help though.

i can confirm that Fabio's patch doesn't fix the clock issue on Duckbill. But the idea seems to be right. If i remove all write access to CLKSEQ and FRAC0 in clk_misc_init() instead of swap them, i don't need the ref_xtal hack anymore. I think Fabio's patch doesn't work because the bootloader (U-Boot in my case) is already feeding SSP from ref_io.

Regards
Stefan

> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Uwe Kleine-König Aug. 8, 2018, 9:09 a.m. UTC | #30
On Wed, Aug 08, 2018 at 10:23:37AM +0200, Stefan Wahren wrote:
> Hi,
> 
> > > On Thu, Jul 26, 2018 at 11:50 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> > > 
> > > > I only want to note that we are using this ugly hack for Duckbill to avoid
> > > > this issue:
> > > >
> > > > https://github.com/I2SE/linux/commit/e457d5e0309c07e30e8dd8b17c32f7b3cbdd9547
> > > 
> > > Do you see the problem on TQMa28L with Stefan's hack applied?
> > 
> > With this change by Stefan the bug is not reproducible. Your change to
> > set the bypass bits only later doesn't help though.
> 
> i can confirm that Fabio's patch doesn't fix the clock issue on
> Duckbill. But the idea seems to be right. If i remove all write access
> to CLKSEQ and FRAC0 in clk_misc_init() instead of swap them, i don't
> need the ref_xtal hack anymore. I think Fabio's patch doesn't work
> because the bootloader (U-Boot in my case) is already feeding SSP from
> ref_io.

I spend some time with this problem again and found out that it also
depends on hclk. If that runs quicker than 2x sspX I couldn't reproduce
the issue. When slowing down hclk to 1x sspX the issue reappeared with
all stuff related to sspX keeping the same.

Probably a problem when the value crosses the clock domain from sspX to
hclk?!

Best regards
Uwe
Uwe Kleine-König March 22, 2019, 9:51 p.m. UTC | #31
On Wed, May 03, 2017 at 08:56:25PM +0200, Uwe Kleine-König wrote:
> Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and
> 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for
> SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized
> to 288 MHz because the initial frequency "seems too high to be ssp clock
> source directly". However this isn't enough to ensure that the frequency
> isn't increased later again. In fact this happens on my machine as the
> mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being
> ssp2 which is resolved to
> 
> 	ref_io1.rate = 320 MHz
> 	ssp2_sel.parent = ref_io1
> 	ssp2_div.divider = 2
> 
> . The observed effect is that reading MISO reliably fails: Instead of
> the least significant bit the second least significant bit is reported
> twice. This is probably related to the reports
> 
> 	https://community.nxp.com/thread/290209
> 	https://community.nxp.com/thread/310434
> 	https://community.nxp.com/thread/385546
> 
> So additionally to initializing ref_io0 and ref_io1 to 288 MHz ensure
> that their frequency is never set to something bigger later on. This is
> done by adding a parameter min_frac to clk-ref and use that instead of
> the hard coded 18 to limit the valid values for FRAC. For all ref clocks
> but ref_io0 and ref_io1 18 is used and so there is no functional change
> for those.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> I'd expect this to go in as a fix as it repairs reading from an EEPROM which
> should be a fairly common task.
> 
> It would be nice to get confirmation from NXP that this is a right fix and
> that spi transfers are supposed to be reliable with ref_ioX <= 288 MHz.

I spend some time on this problem once more. I examined a machine that
showed no issues with CLKCTRL_FRAC0.IO1FRAC >= 19, but reliably reported
more than 32000 read errors with CLKCTRL_FRAC0.IO1FRAC == 18 when
dumping an EEPROM that contains 32768 bytes that can show the error.

The boring to find insights are, that the error is independent of the
other dividers in the clock tree that affect the spi clk frequency and
hbus.

A more interesting insight is that applying coolant spray nearly
halves the read rate (as reported by dd) but also the error rate drops
to 0. After some time the machine reheats again and the error rate
raises to nearly 100% as before.

I just shared that information with NXP's tech support and will keep you
updated on their reactions.

Best regards
Uwe
Uwe Kleine-König May 2, 2019, 12:37 p.m. UTC | #32
Hello,

On Fri, Mar 22, 2019 at 10:51:32PM +0100, Uwe Kleine-König wrote:
> On Wed, May 03, 2017 at 08:56:25PM +0200, Uwe Kleine-König wrote:
> > Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and
> > 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for
> > SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized
> > to 288 MHz because the initial frequency "seems too high to be ssp clock
> > source directly". However this isn't enough to ensure that the frequency
> > isn't increased later again. In fact this happens on my machine as the
> > mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being
> > ssp2 which is resolved to
> > 
> > 	ref_io1.rate = 320 MHz
> > 	ssp2_sel.parent = ref_io1
> > 	ssp2_div.divider = 2
> > 
> > . The observed effect is that reading MISO reliably fails: Instead of
> > the least significant bit the second least significant bit is reported
> > twice. This is probably related to the reports
> > 
> > 	https://community.nxp.com/thread/290209
> > 	https://community.nxp.com/thread/310434
> > 	https://community.nxp.com/thread/385546
> > 
> > So additionally to initializing ref_io0 and ref_io1 to 288 MHz ensure
> > that their frequency is never set to something bigger later on. This is
> > done by adding a parameter min_frac to clk-ref and use that instead of
> > the hard coded 18 to limit the valid values for FRAC. For all ref clocks
> > but ref_io0 and ref_io1 18 is used and so there is no functional change
> > for those.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > Hello,
> > 
> > I'd expect this to go in as a fix as it repairs reading from an EEPROM which
> > should be a fairly common task.
> > 
> > It would be nice to get confirmation from NXP that this is a right fix and
> > that spi transfers are supposed to be reliable with ref_ioX <= 288 MHz.
> 
> I spend some time on this problem once more. I examined a machine that
> showed no issues with CLKCTRL_FRAC0.IO1FRAC >= 19, but reliably reported
> more than 32000 read errors with CLKCTRL_FRAC0.IO1FRAC == 18 when
> dumping an EEPROM that contains 32768 bytes that can show the error.
> 
> The boring to find insights are, that the error is independent of the
> other dividers in the clock tree that affect the spi clk frequency and
> hbus.
> 
> A more interesting insight is that applying coolant spray nearly
> halves the read rate (as reported by dd) but also the error rate drops
> to 0. After some time the machine reheats again and the error rate
> raises to nearly 100% as before.
> 
> I just shared that information with NXP's tech support and will keep you
> updated on their reactions.

The gist of their response is that this CPU is too old and so they don't
have much resources to spend time debugging things there any more. The
supporter suggested to upgrade to i.MX6 or i.MX7 instead ...

So we're living with the work around suggested in this thread and pray
it will continue to work for us even tough it doesn't seem to be robust
enough to fix the same problem for Stefan.

Best regards
Uwe
diff mbox

Patch

diff --git a/drivers/clk/mxs/clk-imx23.c b/drivers/clk/mxs/clk-imx23.c
index f01876af6bb8..b7d3c44bbcbb 100644
--- a/drivers/clk/mxs/clk-imx23.c
+++ b/drivers/clk/mxs/clk-imx23.c
@@ -117,10 +117,10 @@  static void __init mx23_clocks_init(struct device_node *np)
 
 	clks[ref_xtal] = mxs_clk_fixed("ref_xtal", 24000000);
 	clks[pll] = mxs_clk_pll("pll", "ref_xtal", PLLCTRL0, 16, 480000000);
-	clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll", FRAC, 0);
-	clks[ref_emi] = mxs_clk_ref("ref_emi", "pll", FRAC, 1);
-	clks[ref_pix] = mxs_clk_ref("ref_pix", "pll", FRAC, 2);
-	clks[ref_io] = mxs_clk_ref("ref_io", "pll", FRAC, 3);
+	clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll", FRAC, 0, 18);
+	clks[ref_emi] = mxs_clk_ref("ref_emi", "pll", FRAC, 1, 18);
+	clks[ref_pix] = mxs_clk_ref("ref_pix", "pll", FRAC, 2, 18);
+	clks[ref_io] = mxs_clk_ref("ref_io", "pll", FRAC, 3, 18);
 	clks[saif_sel] = mxs_clk_mux("saif_sel", CLKSEQ, 0, 1, sel_pll, ARRAY_SIZE(sel_pll));
 	clks[lcdif_sel] = mxs_clk_mux("lcdif_sel", CLKSEQ, 1, 1, sel_pix, ARRAY_SIZE(sel_pix));
 	clks[gpmi_sel] = mxs_clk_mux("gpmi_sel", CLKSEQ, 4, 1, sel_io, ARRAY_SIZE(sel_io));
diff --git a/drivers/clk/mxs/clk-imx28.c b/drivers/clk/mxs/clk-imx28.c
index 6b572b759f9a..09e324cbd32f 100644
--- a/drivers/clk/mxs/clk-imx28.c
+++ b/drivers/clk/mxs/clk-imx28.c
@@ -174,13 +174,13 @@  static void __init mx28_clocks_init(struct device_node *np)
 	clks[pll0] = mxs_clk_pll("pll0", "ref_xtal", PLL0CTRL0, 17, 480000000);
 	clks[pll1] = mxs_clk_pll("pll1", "ref_xtal", PLL1CTRL0, 17, 480000000);
 	clks[pll2] = mxs_clk_pll("pll2", "ref_xtal", PLL2CTRL0, 23, 50000000);
-	clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll0", FRAC0, 0);
-	clks[ref_emi] = mxs_clk_ref("ref_emi", "pll0", FRAC0, 1);
-	clks[ref_io1] = mxs_clk_ref("ref_io1", "pll0", FRAC0, 2);
-	clks[ref_io0] = mxs_clk_ref("ref_io0", "pll0", FRAC0, 3);
-	clks[ref_pix] = mxs_clk_ref("ref_pix", "pll0", FRAC1, 0);
-	clks[ref_hsadc] = mxs_clk_ref("ref_hsadc", "pll0", FRAC1, 1);
-	clks[ref_gpmi] = mxs_clk_ref("ref_gpmi", "pll0", FRAC1, 2);
+	clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll0", FRAC0, 0, 18);
+	clks[ref_emi] = mxs_clk_ref("ref_emi", "pll0", FRAC0, 1, 18);
+	clks[ref_io1] = mxs_clk_ref("ref_io1", "pll0", FRAC0, 2, 30);
+	clks[ref_io0] = mxs_clk_ref("ref_io0", "pll0", FRAC0, 3, 30);
+	clks[ref_pix] = mxs_clk_ref("ref_pix", "pll0", FRAC1, 0, 18);
+	clks[ref_hsadc] = mxs_clk_ref("ref_hsadc", "pll0", FRAC1, 1, 18);
+	clks[ref_gpmi] = mxs_clk_ref("ref_gpmi", "pll0", FRAC1, 2, 18);
 	clks[saif0_sel] = mxs_clk_mux("saif0_sel", CLKSEQ, 0, 1, sel_pll0, ARRAY_SIZE(sel_pll0));
 	clks[saif1_sel] = mxs_clk_mux("saif1_sel", CLKSEQ, 1, 1, sel_pll0, ARRAY_SIZE(sel_pll0));
 	clks[gpmi_sel] = mxs_clk_mux("gpmi_sel", CLKSEQ, 2, 1, sel_gpmi, ARRAY_SIZE(sel_gpmi));
diff --git a/drivers/clk/mxs/clk-ref.c b/drivers/clk/mxs/clk-ref.c
index 495f99b7965e..9385563b3ea8 100644
--- a/drivers/clk/mxs/clk-ref.c
+++ b/drivers/clk/mxs/clk-ref.c
@@ -23,13 +23,17 @@ 
  *
  * The mxs reference clock sources from pll.  Every 4 reference clocks share
  * one register space, and @idx is used to identify them.  Each reference
- * clock has a gate control and a fractional * divider.  The rate is calculated
+ * clock has a gate control and a fractional divider.  The rate is calculated
  * as pll rate  * (18 / FRAC), where FRAC = 18 ~ 35.
+ *
+ * As some clocks are not reliable at high speed (imx28 io0 and io1), the clock
+ * can be limited to not use FRAC < min_frac.
  */
 struct clk_ref {
 	struct clk_hw hw;
 	void __iomem *reg;
 	u8 idx;
+	u8 min_frac;
 };
 
 #define to_clk_ref(_hw) container_of(_hw, struct clk_ref, hw)
@@ -66,6 +70,7 @@  static unsigned long clk_ref_recalc_rate(struct clk_hw *hw,
 static long clk_ref_round_rate(struct clk_hw *hw, unsigned long rate,
 			       unsigned long *prate)
 {
+	struct clk_ref *ref = to_clk_ref(hw);
 	unsigned long parent_rate = *prate;
 	u64 tmp = parent_rate;
 	u8 frac;
@@ -74,8 +79,8 @@  static long clk_ref_round_rate(struct clk_hw *hw, unsigned long rate,
 	do_div(tmp, rate);
 	frac = tmp;
 
-	if (frac < 18)
-		frac = 18;
+	if (frac < ref->min_frac)
+		frac = ref->min_frac;
 	else if (frac > 35)
 		frac = 35;
 
@@ -99,8 +104,8 @@  static int clk_ref_set_rate(struct clk_hw *hw, unsigned long rate,
 	do_div(tmp, rate);
 	frac = tmp;
 
-	if (frac < 18)
-		frac = 18;
+	if (frac < ref->min_frac)
+		frac = ref->min_frac;
 	else if (frac > 35)
 		frac = 35;
 
@@ -125,7 +130,7 @@  static const struct clk_ops clk_ref_ops = {
 };
 
 struct clk *mxs_clk_ref(const char *name, const char *parent_name,
-			void __iomem *reg, u8 idx)
+			void __iomem *reg, u8 idx, u8 min_frac)
 {
 	struct clk_ref *ref;
 	struct clk *clk;
@@ -143,6 +148,7 @@  struct clk *mxs_clk_ref(const char *name, const char *parent_name,
 
 	ref->reg = reg;
 	ref->idx = idx;
+	ref->min_frac = min_frac;
 	ref->hw.init = &init;
 
 	clk = clk_register(NULL, &ref->hw);
diff --git a/drivers/clk/mxs/clk.h b/drivers/clk/mxs/clk.h
index 5a264a486ad9..fb63462a78cd 100644
--- a/drivers/clk/mxs/clk.h
+++ b/drivers/clk/mxs/clk.h
@@ -28,7 +28,7 @@  struct clk *mxs_clk_pll(const char *name, const char *parent_name,
 			void __iomem *base, u8 power, unsigned long rate);
 
 struct clk *mxs_clk_ref(const char *name, const char *parent_name,
-			void __iomem *reg, u8 idx);
+			void __iomem *reg, u8 idx, u8 min_frac);
 
 struct clk *mxs_clk_div(const char *name, const char *parent_name,
 			void __iomem *reg, u8 shift, u8 width, u8 busy);